[Translation] Allow translation:extract to sort messages with --force
#52938
Conversation
|
Hey! To help keep things organized, we don't allow "Draft" pull requests. Could you please click the "ready for review" button or close this PR and open a new one when you are done? Note that a pull request does not have to be "perfect" or "ready for merge" when you first open it. We just want it to be ready for a first review. Cheers! Carsonbot |
translation:extract to sort messages with --force
0991449 to
ad69499
Compare
|
Ok so draft PRs are not allowed so I made a quick attempt to my idea :) It seems to work, Also, I'm really not sure about the impact of my change on the different dumpers (especially |
This commit reverts the change made in PR symfony#49189 as the command accepts the argument `--sort` with `--force`. Reference: symfony#49189
To not introduce a breaking change in the API of the translation:extract command, I need to accept the "null" value for the `--sort` option when `--force` is passed to the command. Meaning that I had to remove the default value of the `InputOption`. In the mean time, I didn't want to break the previous behaviour when using `--dump-messages`. So in this case, when the option is not passed, I fallback to the value "asc".
| krsort($messages); | ||
| } elseif ('asc' === $sort) { | ||
| ksort($messages); | ||
| } |
There was a problem hiding this comment.
Should the method throw an error if the value of sort is invalid? I like not being too strict and doing nothing when the value is different, but it's less clear if a typo is made (e.g. passing the value acs instead of asc)
| @@ -87,7 +87,7 @@ protected function configure(): void | |||
| new InputOption('force', null, InputOption::VALUE_NONE, 'Should the extract be done'), | |||
| new InputOption('clean', null, InputOption::VALUE_NONE, 'Should clean not found messages'), | |||
| new InputOption('domain', null, InputOption::VALUE_OPTIONAL, 'Specify the domain to extract'), | |||
| new InputOption('sort', null, InputOption::VALUE_OPTIONAL, 'Return list of messages sorted alphabetically (only works with --dump-messages)', 'asc'), | |||
| new InputOption('sort', null, InputOption::VALUE_OPTIONAL, 'Return list of messages sorted alphabetically'), | |||
There was a problem hiding this comment.
I'm a bit unsure about removing the default value. As explained in my commit, it's to allow "null" value in order to keep the behaviour from before when using --force. I made sure to default to "asc" when using --dump-messages, but I may miss some issues.
|
|
||
| return json_encode($messages->all($domain), $flags); | ||
| return json_encode($messages->all($domain, $sort), $flags); |
There was a problem hiding this comment.
I don't know what to do with the Psalm errors on this line. They are not related to my PR so I would say that I should not change anything here. What do you say?
|
Hi @welcoMattic 👋 I saw that you were assigned as reviewer on my PR :) I made some changes: everything seems to work to me, but I'm a bit unsure about the whole decision of passing a sort option to the dumpers (it seems ok to me, but I'm not familiar with the code of Symfony). Also I added some questions in specific comments. No urgency: I will probably not be able to work again on this code again before the end of January. This is just to let you know that the PR looks correct to me, but that I might need some help! |
|
Similar to #52337 |
|
IIUC, this has been fixed in #58408 |
Fix #49250
Similar to #52337
I've started to work on #37918 in order to sort the translations when using the
--forceargument of the commandtranslation:extract(previouslytranslation:update).Actually, sorting only works when using
--dump-messages. However, it's pretty common that developers want to keep their translations sorted by alphabetical order in order to navigate more easily in the translations files. Actually, this has to be done manually, increasing the risk of mistakes.At the moment, my PR only adds sorting to theImplemented, see my message below :)MessageCatalogue::all()method. I'm not sure how to pass the option from the command down to the MessageCatalogue and I'll need help on that. My best guess is to pass the sort value as a new option to the writer in theTranslationUpdateCommandclass. Then, the writer pass the options to the dumper, which is in charge of callingMessageCatalogue::all()with the sort option. It means making sure to amend all the different dumpers, but it seems doable.Does this solution look correct to you?