Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

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

Sign up
Appearance settings

[Console] Fix commands description with numeric namespaces#34130

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

Conversation

@fancyweb
Copy link
Contributor

@fancywebfancyweb commentedOct 26, 2019
edited
Loading

QA
Branch?3.4
Bug fix?yes
New feature?no
Deprecations?no
Tickets#34111
LicenseMIT
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 with0: (cf'' === comparison).

@Bilge
Copy link
Contributor

Bilge commentedOct 31, 2019
edited
Loading

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
ContributorAuthor

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 theSORT_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 commentedNov 1, 2019
edited
Loading

I doubt this is going to break someone workflow

I was trying not to make this about me, but the implication was that it breaksmy 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 withz or some other late-sorted character to achieve the same result.

Copy link
Member

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

Bilge reacted with heart emoji
@fancywebfancywebforce-pushed theconsole-fix-numeric-command-ns-description branch 2 times, most recently fromf7dc8bf to2ef5028CompareNovember 7, 2019 09:10
@nicolas-grekas
Copy link
Member

Could you please add a test case?

@fancywebfancywebforce-pushed theconsole-fix-numeric-command-ns-description branch from2ef5028 to4d47868CompareNovember 28, 2019 13:21
@fancyweb
Copy link
ContributorAuthor

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. Seehttps://3v4l.org/SJcMB.

@nicolas-grekas
Copy link
Member

Thank you@fancyweb.

nicolas-grekas added a commit that referenced this pull requestNov 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-grekasnicolas-grekas merged commit4d47868 intosymfony:3.4Nov 28, 2019
@fancywebfancyweb deleted the console-fix-numeric-command-ns-description branchNovember 28, 2019 13:29
This was referencedDec 1, 2019
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

3.4

Development

Successfully merging this pull request may close these issues.

4 participants

@fancyweb@Bilge@nicolas-grekas@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp