Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Translator] Default value for 'sort' option in translation:update should be 'asc'#35486
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Nyholm left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thank you for this PR.
I dont see where you set the default value. Am I missing something?
| return1; | ||
| if ($input->hasOption('sort')) { | ||
| if ($sort =$input->getOption('sort')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can this ever be false?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
getOption() returns null if no value has been defined.
Line 296: if thesort option has been defined, even if it has no value.
Lines 297-304: if a value has been defined, check that it's valid
Line 307: if the option has been explicitly set to 'desc', makes an rsort()
Line 398: we do a sort() in all other cases (including if there's no value defined)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why not defining the default value on the option definition instead of dealing with null and only allow asc and desc as strings and no empty value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm ok with this technicaly, but it would change the default behavior, where--sort=asc would be applied even if the--sort option is not specified. I didn't get into this because the goal is just to fix a bug, and I don't know if the default order responds to any particular logic.
There are actually three possible states: no sorting, asc and desc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would also setASC as the default value of the option instead of allowingnull which does not mean anything.
| return1; | ||
| if ($input->hasOption('sort')) { | ||
| if ($sort =$input->getOption('sort')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would also setASC as the default value of the option instead of allowingnull which does not mean anything.
versgui commentedJan 30, 2020
I made the modification: the command now returns the list with "sort=asc" by default. |
Nyholm left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thank you.
Looking atif ($sort = $input->getOption('sort')), it is fine to keep the if statement. If someone calls with--sort=, we will be safe.
0b6a58c tofdb13c8Comparefabpot commentedJan 30, 2020
Thank you@versgui. |
…n:update should be 'asc' (versgui)This PR was squashed before being merged into the 4.4 branch (closes#35486).Discussion----------[Translator] Default value for 'sort' option in translation:update should be 'asc'| Q | A| ------------- | ---| Branch? | 4.4| Bug fix? | yes| New feature? | no| Deprecations? | no| Tickets || License | MIT| Doc PR |The value for 'sort' option for `bin/console translation:update --sort` is optional, but no default value is defined. So the list isn't sorted if no value is explicitly defined.This MR brings a default value "asc" if no value is defined, so the list is correctly sorted.Commits-------fdb13c8 [Translator] Default value for 'sort' option in translation:update should be 'asc'
The value for 'sort' option for
bin/console translation:update --sortis optional, but no default value is defined. So the list isn't sorted if no value is explicitly defined.This MR brings a default value "asc" if no value is defined, so the list is correctly sorted.