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

[FrameworkBundle][TranslationDebug] Return non-zero exit code on failure #29139

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@DAcodedBEAT
Copy link

commented Nov 8, 2018

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets n/a
License MIT
Doc PR TBD

This PR introduces non-zero exit codes upon failure for the debug:translations command. The addition can be useful for projects which wish to determine results easily in CI.

The exit code returned can be interpreted as a bit array and to determine what failed, one could Bitwise AND the returned exit code to determine what failed.

Exit Codes:

Failure Reason bit array integer
General Failure (i.e no translations at all) 00001 1
Missing Translations 00010 2
Unused Translations 00100 4
Fallback Translations 01000 8

Example: Given there are missing and unused translations, the expected status code would be TranslationDebugCommand::EXIT_CODE_MISSING | TranslationDebugCommand::EXIT_CODE_UNUSED, which would be evaluate to 6.

@DAcodedBEAT DAcodedBEAT force-pushed the DAcodedBEAT:debug_translation_add_strict_option branch from 7965ed3 to 0e4e925 Nov 8, 2018

@nicolas-grekas nicolas-grekas added this to the next milestone Nov 8, 2018

@DAcodedBEAT

This comment has been minimized.

Copy link
Author

commented Nov 13, 2018

Hi everyone! First time symfony contributor here. It’d be super swell to get this merged in. What do I need to do to get some code review for this?

@DAcodedBEAT

This comment has been minimized.

Copy link
Author

commented Nov 20, 2018

@nicolas-grekas @fabpot what should I do to expedite the process of merging this in?

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Nov 20, 2018

We're in feature freeze finalizing version 4.2. we'll start merging new features again in two to three weeks (after/during SymfonyCon Lisbon I'd say.)
Thanks for your patience.

@@ -81,6 +84,7 @@ protected function configure()
new InputOption('only-missing', null, InputOption::VALUE_NONE, 'Displays only missing messages'),
new InputOption('only-unused', null, InputOption::VALUE_NONE, 'Displays only unused messages'),
new InputOption('all', null, InputOption::VALUE_NONE, 'Load messages from all registered bundles'),
new InputOption('strict', null, InputOption::VALUE_OPTIONAL, 'Returns a non-zero exit code upon failure', false),

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Dec 13, 2018

Member

at first, the description made me think this was a boolean option.
what about --fail-on-missing etc (would be more flexible also)?

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

Not a big fan of adding a new flag for that. I think not returning a non-zero exit code was a mistake and should be done without a flag.

@DAcodedBEAT DAcodedBEAT force-pushed the DAcodedBEAT:debug_translation_add_strict_option branch from 0e4e925 to ed56b0d Mar 6, 2019

@DAcodedBEAT

This comment has been minimized.

Copy link
Author

commented Mar 6, 2019

Hello @fabpot and @nicolas-grekas! Apologies for the delay. Originally I thought it was silly to not return a non-zero exit code by default as well, but then I realized that this command could be used for fallback messages. Because of this, knowing when to return a non-zero status code could be difficult (e.g. should the command return a non-zero exit code due to missing a localized translation or should it return a successful zero exit code because a fallback could be used instead).

When initially writing the this PR, I pulled inspiration from composer's outdated command and the ability to increase verbosity levels in numerous commands (-v vs -vvv), but I could definitely be convinced to pivot to an alternative. 🙂

@DAcodedBEAT DAcodedBEAT force-pushed the DAcodedBEAT:debug_translation_add_strict_option branch 2 times, most recently from e1b45e9 to 990dbc0 May 28, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented Jul 8, 2019

I still think that this should not be configurable via an option. The return code could be different based on the kind of errors though.

@DAcodedBEAT DAcodedBEAT force-pushed the DAcodedBEAT:debug_translation_add_strict_option branch from 990dbc0 to 3a9f7c3 Jul 11, 2019

@DAcodedBEAT DAcodedBEAT changed the title [FrameworkBundle][TranslationDebug] add strict option [FrameworkBundle][TranslationDebug] Return non-zero exit code on failure Jul 11, 2019

@DAcodedBEAT

This comment has been minimized.

Copy link
Author

commented Jul 11, 2019

@fabpot I have changed this PR to return a non-zero exit code on failure. Can you please re-review this?
@nicolas-grekas Given these turn of events, would you still want these --fail-on-* options added to this PR?

Thank you both for your feedback 🙂

const EXIT_CODE_GENERAL_ERROR = 1;
const EXIT_CODE_MISSING = 2;
const EXIT_CODE_UNUSED = 4;
const EXIT_CODE_FALLBACK = 8;

This comment has been minimized.

Copy link
@fabpot

fabpot Jul 12, 2019

Member

Console exit codes are kind of standardized (see https://www.tldp.org/LDP/abs/html/exitcodes.html). I propose to use 64 and above here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.