Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Console] Added suggest on bad command name #3325

Merged
merged 5 commits into from Feb 14, 2012

Conversation

lyrixx
Copy link
Member

@lyrixx lyrixx commented Feb 11, 2012

Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: namespace ?

Added something like in git : if user type a wrong command and if a close alternative exists, Command compenent will display a list of similar command(s).

Note : It does not work with namespace. If this PR will be merged, I could work on namespace.

see : https://github.com/fabpot/Twig/blob/master/lib/Twig/Environment.php#L1003

$message = sprintf('Command "%s" is not defined.', $name);

if ($alternatives = $this->findAlternativeCommands($searchName)) {
$message .= PHP_EOL.'Did you mean one of these?'.PHP_EOL.' ';
Copy link
Member

Choose a reason for hiding this comment

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

There is no need to use PHP_EOL here (\n is fine).

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I Fixed it.

@fabpot
Copy link
Member

fabpot commented Feb 11, 2012

I think we need it to also work on namespace before merging. Is it possible?

@henrikbjorn
Copy link
Contributor

could maybe use similar_text ?

@lyrixx
Copy link
Member Author

lyrixx commented Feb 11, 2012

Yes.
I will work on it asap

@lyrixx
Copy link
Member Author

lyrixx commented Feb 11, 2012

I added code for namespace

@henrikbjorn I did the same logic as in twig.

@lyrixx
Copy link
Member Author

lyrixx commented Feb 11, 2012

Note : Travis tests failed : http://travis-ci.org/#!/lyrixx/symfony/builds/663216
before_script: Execution of 'php vendors.php' took longer than 600 seconds and was terminated. Consider rewriting your stuff in AssemblyScript, we've heard it handles Web Scale™

But tests are OK on my laptop

@stof
Copy link
Member

stof commented Feb 11, 2012

Well, it may be due to github issues during the setup of the vendors. There is some issues regularly because of the DDoS attack.

@lyrixx
Copy link
Member Author

lyrixx commented Feb 11, 2012

Yes, i guessed it :-) that's why i notice it work on my laptop

@fabpot
Copy link
Member

fabpot commented Feb 11, 2012

This code won't work if you use abbreviations instead of the full namespace or command name.

@lyrixx
Copy link
Member Author

lyrixx commented Feb 12, 2012

I added code to manage abbreviations. But I'm not sure what you are expecting. Can you try it and give me some feedback ?

P.S. : Travis failed again, but tests pass on my laptop.

fabpot added a commit that referenced this pull request Feb 14, 2012
Commits
-------

e5edf5a [Console] Fixed CS
8abf506 [Console] Added abbreviation into search for bad command / namespace
c6203bc [Console] Added namespace suggest on bad namespace name
117359a [Console] fixed CS according to PR comment
dd0d97e [Console] Added suggest on bad command name

Discussion
----------

[Console] Added suggest on bad command name

Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: namespace ?

Added something like in `git` :  if user type a wrong command and if a close alternative exists, Command compenent will display a list of similar command(s).

Note : It does not work with namespace. If this PR will be merged, I could work on namespace.

see : https://github.com/fabpot/Twig/blob/master/lib/Twig/Environment.php#L1003

---------------------------------------------------------------------------

by fabpot at 2012-02-11T18:54:49Z

I think we need it to also work on namespace before merging. Is it possible?

---------------------------------------------------------------------------

by henrikbjorn at 2012-02-11T19:01:06Z

could maybe use similar_text ?

---------------------------------------------------------------------------

by lyrixx at 2012-02-11T19:01:55Z

Yes.
I will work on it asap

---------------------------------------------------------------------------

by lyrixx at 2012-02-11T20:06:43Z

I added code for namespace

@henrikbjorn I did the same logic as in twig.

---------------------------------------------------------------------------

by lyrixx at 2012-02-11T20:27:48Z

Note : Travis tests failed : http://travis-ci.org/#!/lyrixx/symfony/builds/663216
```before_script: Execution of 'php vendors.php' took longer than 600 seconds and was terminated.
Consider rewriting your stuff in AssemblyScript, we've heard it handles Web Scale™```

But tests are OK on my laptop

---------------------------------------------------------------------------

by stof at 2012-02-11T20:41:15Z

Well, it may be due to github issues during the setup of the vendors. There is some issues regularly because of the DDoS attack.

---------------------------------------------------------------------------

by lyrixx at 2012-02-11T20:58:07Z

Yes, i guessed it :-) that's why i notice it work on my laptop

---------------------------------------------------------------------------

by fabpot at 2012-02-11T23:11:08Z

This code won't work if you use abbreviations instead of the full namespace or command name.

---------------------------------------------------------------------------

by lyrixx at 2012-02-12T23:30:04Z

I added code to manage abbreviations. But I'm not sure what you are expecting. Can you try it and give me some feedback ?

P.S. : Travis failed again, but tests pass on my laptop.
@fabpot fabpot merged commit e5edf5a into symfony:master Feb 14, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants