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

Open
wants to merge 1 commit into
base: 3.4
from

Conversation

@fancyweb
Copy link
Contributor

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor Author

fancyweb commented Oct 31, 2019

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

This comment has been minimized.

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 left a comment

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 fancyweb:console-fix-numeric-command-ns-description branch from 2340205 to f7dc8bf Nov 5, 2019
@fancyweb fancyweb force-pushed the fancyweb:console-fix-numeric-command-ns-description branch from f7dc8bf to 2ef5028 Nov 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.