NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

Sort alternatives alphabetically when a command is not found#19887

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

fabpot merged 2 commits intosymfony:masterfromjaviereguiluz:fix_10893

Mar 22, 2017

Merged

Sort alternatives alphabetically when a command is not found#19887

fabpot merged 2 commits intosymfony:masterfromjaviereguiluz:fix_10893

Mar 22, 2017

Conversation

Copy link

Member

javiereguiluz commentedSep 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	-

javiereguiluz added Feature Console labels

Sep 8, 2016

carsonbot added the Status: Needs Review label

Sep 8, 2016

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

That's probably because of the: when sorting

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I do understand that, my point is that we need to take care of that: when sorting like a human would do.

Copy link

MemberAuthor

javiereguiluzSep 14, 2016•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 useproject:,project1: orapp:,app1: namespaces.

Copy link

Member

fabpotSep 14, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

fair enough

Copy link

Member

fabpot commentedSep 14, 2016

Have you had time to have a look at the other exceptions using alternatives to see if sorting them would also make sense?

Copy link

Contributor

ro0NL commentedSep 17, 2016•
edited
Loading

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.bin/console vs.bin/console keyword render the same output, but the latter only shows matching commands and some little info like"Showing 1 of 3 commands matching 'keyword'".

Copy link

Member

fabpot commentedNov 9, 2016

@javiereguiluz Any news?

nicolas-grekas added this to the3.x milestone

Dec 6, 2016

Copy link

Member

fabpot commentedMar 1, 2017

@javiereguiluz Still interested in finishing this PR?

javiereguiluz self-assigned this

Mar 1, 2017

Copy link

MemberAuthor

javiereguiluz commentedMar 1, 2017

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!

Copy link

Member

fabpot commentedMar 1, 2017

I think looking forlevenshtein in the code base would give you all the places where we need to make the same change.

Copy link

MemberAuthor

javiereguiluz commentedMar 6, 2017

I've committed some changes:

For the list of commands, I think that sorting best matches alphabetically is the most natural thing...
... but for the other levenshtein candidates (service ids, param names, etc.) I propose to sort them from best to worst (i.e. from lowest to highest levenshtein distance)

javiereguiluz added2 commits

March 6, 2017 11:28

Sort alternatives alphabetically when a command is not found

f04b1bd

Sort commands like a human would do

ba6c946

javiereguiluz force-pushed thefix_10893 branch fromac4f97b to5ebd03cCompare

March 6, 2017 10:30

fabpot reviewed

Mar 6, 2017

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/UnusedTagsPass.php Outdated

		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;

Copy link

Member

fabpotMar 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

why not just$candidates[$definedTag] = true;? That would avoid creating the$lev variable that is not used anyway.

Copy link

MemberAuthor

javiereguiluzMar 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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);

Copy link

Member

fabpotMar 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Copy link

Member

fabpot commentedMar 14, 2017

Tests fail on deps=low, we should find a way to fix that.

Copy link

MemberAuthor

javiereguiluz commentedMar 19, 2017

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:

Did you mean one of these: "bar", "baz"?

But after this PR, that fails on most Symfony versions, so I changed it to:

Did you mean one of these: "baz", "bar"?

and now test pass ... except when using "deps = low", which fails because the expected output is again:

Did you mean one of these: "bar", "baz"?

Copy link

Contributor

linaori commentedMar 19, 2017

@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.

Copy link

MemberAuthor

javiereguiluz commentedMar 22, 2017

After thinking about this, what if we do the following:

Remove the last 3 commits
Keep only the original feature that sorts commands alphabetically

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?

Copy link

Member

fabpot commentedMar 22, 2017

Make sense 👍

javiereguiluz force-pushed thefix_10893 branch from5ebd03c toba6c946Compare

March 22, 2017 20:35

Copy link

Member

fabpot commentedMar 22, 2017

Thank you@javiereguiluz.

fabpot merged commitba6c946 intosymfony:master

Mar 22, 2017

fabpot added a commit that referenced this pull request

Mar 22, 2017

feature#19887Sort alternatives alphabetically when a command is not…

0a17358

… 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 dof04b1bd Sort alternatives alphabetically when a command is not found

nicolas-grekas modified the milestones:3.x,3.3

Mar 24, 2017

nicolas-grekas removed this from the3.x milestone

Mar 24, 2017

fabpot mentioned this pull request

May 1, 2017

Release v3.3.0-BETA1#22603

Merged