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] Fix commands description with numeric namespaces #34130

Conversation

fancyweb
Copy link
Contributor

@fancyweb fancyweb commented Oct 26, 2019

Q A
Branch? 3.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets #34111
License MIT
Doc PR -

This PR fixes the linked ticket case.

It also changes the keys sorting to display the numeric namespaces first.

It also fixes another bug if your command name starts with _global:. In this special case the command is considered global but its full name is still _global:xxx. We can't do better without more refactoring since the final array of namespaces and global commands is shared, _global just being a special key. Currently, if your command starts with _global, all global commands are not displayed at all so it's better like this anyway.

It also fixes another bug if your command starts with 0: (cf '' === comparison).

@Bilge
Copy link
Contributor

Bilge commented Oct 31, 2019

It also changes the keys sorting to display the numeric namespaces first.

Why not just stick to fixing the bug? It's not strictly a bug that numeric entries come before or after alphabet entries; that's a personal preference and probably breaks someone's workflow.

@fancyweb
Copy link
Contributor Author

This bug proves that numeric namespaces were not taken into account when the code was written. Therefore it's a good occasion to improve their support in addition to purely fixing it.

Displaying the numeric namespaces first is a change that makes sense to me, not because it's a personal preference but because comparing items as strings with the SORT_STRING flag seems the best option since we are displaying strings. Turns out numerics come before letters with this flag...

I doubt this is going to break someone workflow since numeric namespaces are currently not displayed correctly at all (except 0). By fixing the bug, we are already changing the output anyway. Also, I doubt we have a BC break policy on commands output.

Of course, this change is a proposition that I will gladly revert if it is considered useless / bad / not backward compatible by a majority.

@Bilge
Copy link
Contributor

Bilge commented Nov 1, 2019

I doubt this is going to break someone workflow

I was trying not to make this about me, but the implication was that it breaks my workflow. I like my custom commands to appear at the end of the list so they don't always scroll off the top of the terminal when listing commands. Using numeric namespaces currently accomplishes this, but with your proposed change, one would have to rename everything to begin with z or some other late-sorted character to achieve the same result.

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Let's keep the order @Bilge is used to if possible, that's 3.4, it should remain as stable as possible.

@fancyweb fancyweb force-pushed the console-fix-numeric-command-ns-description branch 2 times, most recently from f7dc8bf to 2ef5028 Compare November 7, 2019 09:10
@nicolas-grekas
Copy link
Member

Could you please add a test case?

@fancyweb fancyweb force-pushed the console-fix-numeric-command-ns-description branch from 2ef5028 to 4d47868 Compare November 28, 2019 13:21
@fancyweb
Copy link
Contributor Author

I added a test. There is one edge case we don't support: when you have "0" as the command namespace, its sorted order depends of its processing order. See https://3v4l.org/SJcMB.

@nicolas-grekas
Copy link
Member

Thank you @fancyweb.

nicolas-grekas added a commit that referenced this pull request Nov 28, 2019
… (fancyweb)

This PR was merged into the 3.4 branch.

Discussion
----------

[Console] Fix commands description with numeric namespaces

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | #34111
| License       | MIT
| Doc PR        | -

This PR fixes the linked ticket case.

It also changes the keys sorting to display the numeric namespaces first.

It also fixes another bug if your command name starts with `_global:`. In this special case the command is considered global but its full name is still `_global:xxx`. We can't do better without more refactoring since the final array of namespaces and global commands is shared, `_global` just being a special key. Currently, if your command starts with `_global`, all global commands are not displayed at all so it's better like this anyway.

It also fixes another bug if your command starts with `0:` (cf `'' ===` comparison).

Commits
-------

4d47868 [Console] Fix commands description with numeric namespaces
@nicolas-grekas nicolas-grekas merged commit 4d47868 into symfony:3.4 Nov 28, 2019
@fancyweb fancyweb deleted the console-fix-numeric-command-ns-description branch November 28, 2019 13:29
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.

None yet

4 participants