Sort alternatives alphabetically when a command is not found #19887
Conversation
javiereguiluz
commented
Sep 8, 2016
| Q | A |
|---|---|
| Branch? | master |
| Bug fix? | no |
| New feature? | yes |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #10893 |
| License | MIT |
| Doc PR | - |
|
As we are listing alternatives in a lot of different places in the framework, wouldn't it make sense to change the sort for all of them? |
| 'afoobar', | ||
| 'afoobar1', | ||
| 'afoobar2', | ||
| 'foo1:bar', |
There was a problem hiding this comment.
I would have expected foo1:bar to be after foo:bar as intuitively, I sort the namespace first, then the command name. Not sure if this is something we need to fix though.
There was a problem hiding this comment.
That's probably because of the : when sorting
There was a problem hiding this comment.
I do understand that, my point is that we need to take care of that : when sorting like a human would do.
There was a problem hiding this comment.
The good thing is that in real apps we probably won't have this problem. This example (foo:, foo1:, foo3:) is too abstract and probably our users don't use project:, project1: or app:, app1: namespaces.
|
Have you had time to have a look at the other exceptions using alternatives to see if sorting them would also make sense? |
|
I thought of this yesterday, and it's more or less related; wouldn't it be nice to filter the command list if a command is not found? Ie. |
|
@javiereguiluz Any news? |
|
@javiereguiluz Still interested in finishing this PR? |
|
Yes ... but I don't know where to look for more places where this feature could be implemented. Anyone can give me a clue? Thanks! |
|
I think looking for |
|
I've committed some changes:
|
| if (false !== strpos($definedTag, $tag) || levenshtein($tag, $definedTag) <= strlen($tag) / 3) { | ||
| $candidates[] = $definedTag; | ||
| if (false !== strpos($definedTag, $tag) || $lev = levenshtein($tag, $definedTag) <= strlen($tag) / 3) { | ||
| $candidates[$definedTag] = $lev; |
There was a problem hiding this comment.
why not just $candidates[$definedTag] = true;? That would avoid creating the $lev variable that is not used anyway.
There was a problem hiding this comment.
I'm not sure about this. $lev is the score ... and we need that for the two new lines added below to sort results from best to worst:
asort($candidates);
$candidates = array_keys($candidates);|
Tests fail on deps=low, we should find a way to fix that. |
|
I can't see which is the error that makes test fail for "deps = low". Can anyone help me? Thanks! Some context --> The original test was: But after this PR, that fails on most Symfony versions, so I changed it to: and now test pass ... except when using "deps = low", which fails because the expected output is again: |
|
@javiereguiluz my guess is that the DependencyInjection needs a higher console version for that as it seems like it's the DependencyInjection component that fails. I could be wrong though, but that's my hunch after seeing the failure. |
|
After thinking about this, what if we do the following:
Reason: We already show very good alternatives because we limit the levenhstein cost to < 3, so every alternative displayed is almost as good as the other ones. The proposed changes wouldn't make the results much better. What do you think? |
|
Make sense 👍 |
5ebd03c to
ba6c946
Compare
|
Thank you @javiereguiluz. |
… found (javiereguiluz) This PR was squashed before being merged into the 3.3-dev branch (closes #19887). Discussion ---------- Sort alternatives alphabetically when a command is not found | Q | A | | --- | --- | | Branch? | master | | Bug fix? | no | | New feature? | yes | | BC breaks? | no | | Deprecations? | no | | Tests pass? | yes | | Fixed tickets | #10893 | | License | MIT | | Doc PR | - | Commits ------- ba6c946 Sort commands like a human would do f04b1bd Sort alternatives alphabetically when a command is not found