Skip to content

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

[Console] Made console command shortcuts case insensitive #23869

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

Merged
merged 1 commit into from
Aug 12, 2017
Merged

[Console] Made console command shortcuts case insensitive #23869

merged 1 commit into from
Aug 12, 2017

Conversation

thanosp
Copy link
Contributor

@thanosp thanosp commented Aug 11, 2017

Q A
Branch? 3.4
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #...
License MIT
Doc PR symfony/symfony-docs#...

This patch would save a lot of time wasted correcting typos.
Symfony commands are using : as a namespace separator.
Most keyboards require pressing shift to get this character.
As an accident a developer in hurry (me) when trying to use the shortcut of commands will often type the character after : also while pressing shift. Right now this will lead to an error effectively wasting the attempt to save time by using the shortcut.

e.g. bin/console c:C

@javiereguiluz
Copy link
Member

I don't know if we should make this change (at the moment we're doing the opposite in other parts of the framework: stop being case-insensitive) but I can confirm that this is a very annoying issue. I make this error almost every day. In my case the error is: server:Start instead of server:start 😡

@thanosp
Copy link
Contributor Author

thanosp commented Aug 11, 2017

I agree being strict is perfect for bots and automated stuff. Bots will/should not use shortcuts though. This is purely good for DX that I believe is very important. I don't believe it should be in the same bucket as other more restrictive flows

@fabpot
Copy link
Member

fabpot commented Aug 11, 2017

👍 for this idea, even if I've never hit such an issue myself.

@chalasr
Copy link
Member

chalasr commented Aug 11, 2017

This is a BC break. Taking the same example as in your test case:

// test.php
$app = new Symfony\Component\Console\Application();
$app->register('FOO:BAR')->setCode(function () { print 'FOO:BAR executed'; });
$app->register('foo:bar')->setCode(function () { print 'foo:bar executed'; });
$app->run();

Current behavior is:

$ php test.php F:B
FOO:BAR executed

$ php test.php f:b
foo:bar executed

With this, both f:b and F:B become ambiguous and no one runs, so 👎 for this PR as is.

If we want to fix this issue, we should make command names case insensitive (as @javiereguiluz seems to be asking for), not only shortcuts, and that would require a proper upgrade path.
I'm not opposed to that idea either, but I wonder if we are not missing some valid use cases for case-sensitive command names.

@chalasr chalasr added this to the 3.4 milestone Aug 11, 2017
@chalasr chalasr requested a review from ogizanagi August 11, 2017 17:47
@thanosp
Copy link
Contributor Author

thanosp commented Aug 11, 2017

@chalasr I agree this is a BC break as you stated it. I've updated the table to mark it as such.
This will break functionality for people explicitly choosing to use case sensitive shortcuts by matching more than one commands (not choosing an unexpected one) and refusing to run.
Therefore it would require either more characters to match uniquely or the full command for precision.

But if we assume that such a person exists that prefers f:b, f:B and F:B to match different things, for them I would say having to type : is an even bigger problem. Or they've never noticed that they've been running the wrong command. I would say allowing f:b and f:B pointing to different commands is already bad as too error prone.

I do like the idea of having all commands case insensitive. That would also solve the problem but I don't know how much work that is.

There was a nice talk by @ellotheth called Building for Utopia at PHPTek talking about similar issues.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Aug 11, 2017 via email

@thanosp
Copy link
Contributor Author

thanosp commented Aug 11, 2017

@nicolas-grekas I think we can. I will try that

@thanosp
Copy link
Contributor Author

thanosp commented Aug 11, 2017

Ready for review again.

@chalasr
Copy link
Member

chalasr commented Aug 12, 2017

Could you add a note in the Console CHANGELOG?

@thanosp
Copy link
Contributor Author

thanosp commented Aug 12, 2017

Thanks @chalasr I've updated the Console CHANGELOG

@chalasr
Copy link
Member

chalasr commented Aug 12, 2017

Thank you for contributing to Symfony @thanosp.

@chalasr chalasr merged commit 04df283 into symfony:3.4 Aug 12, 2017
chalasr pushed a commit that referenced this pull request Aug 12, 2017
…ive (thanosp)

This PR was merged into the 3.4 branch.

Discussion
----------

[Console] Made console command shortcuts case insensitive

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | yes <!-- don't forget updating src/**/CHANGELOG.md files -->
| BC breaks?    | no
| Deprecations? | no <!-- don't forget updating UPGRADE-*.md files -->
| Tests pass?   | yes
| Fixed tickets | #... <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!--highly recommended for new features-->

<!--
- Bug fixes must be submitted against the lowest branch where they apply
  (lowest branches are regularly merged to upper ones so they get the fixes too).
- Features and deprecations must be submitted against the 3.4,
  legacy code removals go to the master branch.
- Please fill in this template according to the PR you're about to submit.
- Replace this comment by a description of what your PR is solving.
-->

This patch would save a lot of time wasted correcting typos.
Symfony commands are using `:` as a namespace separator.
Most keyboards require pressing shift to get this character.
As an accident a developer in hurry (me) when trying to use the shortcut of commands will often type the character after `:` also while pressing shift. Right now this will lead to an error effectively wasting the attempt to save time by using the shortcut.

e.g. `bin/console c:C`

Commits
-------

04df283 [Console] Added a case-insensitive fallback for console command names
@thanosp thanosp deleted the console-case-insensitive-shortcuts branch August 14, 2017 07:04
This was referenced Oct 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.