-
Notifications
You must be signed in to change notification settings - Fork 93
Improve download command #369
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
Improve download command #369
Conversation
| ) | ||
| ->addArgument('configuration', InputArgument::OPTIONAL, 'The configuration to use', 'default') | ||
| ->addOption('cache', null, InputOption::VALUE_NONE, 'Clear the cache if the translations have changed') | ||
| ->addOption('cache', null, InputOption::VALUE_NONE, '[DEPRECATED] Cache is now automatically cleared when translations have changed.') |
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.
WDYT about this?
Command/DownloadCommand.php
Outdated
| if ($input->getOption('cache')) { | ||
| $message = 'The --cache option is deprecated as it\'s now the default behaviour of this command.'; | ||
|
|
||
| $io->error($message); |
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.
Is it relevant to display an error or is the trigger_error call enough?
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.
Good question, especially as it's on CLI.
Maybe use note? To be sure return code isn't affected?
https://symfony.com/doc/current/console/style.html#admonition-methods
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.
The return code is never affected by what you display in the CLI. :)
With a note:

Wit a caution:

With an error:

My question was: should we display something in the cli regarding the deprecated option or is the trigger_error call enough?
I used a note as suggested cause it's less "violent" than a caution or an error but we still have the information directly in the cli which is relevant IMO.
da2c93a to
46e2754
Compare
Command/DownloadCommand.php
Outdated
| if ($input->getOption('cache')) { | ||
| $message = 'The --cache option is deprecated as it\'s now the default behaviour of this command.'; | ||
|
|
||
| $io->error($message); |
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.
Good question, especially as it's on CLI.
Maybe use note? To be sure return code isn't affected?
https://symfony.com/doc/current/console/style.html#admonition-methods
46e2754 to
eaf8af4
Compare
Fix #111