Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 0 commits into from
Jan 31, 2020
Merged

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

merged 0 commits into from
Jan 31, 2020

Conversation

DAcodedBEAT
Copy link
Contributor

@DAcodedBEAT DAcodedBEAT commented Nov 8, 2018 •
edited
Loading

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) 1000000 64
Missing Translations 1000001 65
Unused Translations 1000010 66
Fallback Translations 1000100 68

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 evaluated to 67.

@DAcodedBEAT DAcodedBEAT force-pushed the debug_translation_add_strict_option branch from 7965ed3 to 0e4e925 Compare November 8, 2018 13:42
@nicolas-grekas nicolas-grekas added this to the next milestone Nov 8, 2018
@DAcodedBEAT
Copy link
Contributor Author

DAcodedBEAT commented Nov 13, 2018 •
edited
Loading

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
Copy link
Contributor Author

DAcodedBEAT commented Nov 20, 2018 •
edited
Loading

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

@nicolas-grekas
Copy link
Member

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.

nicolas-grekas
nicolas-grekas reviewed Dec 13, 2018
View reviewed changes
src/Symfony/Bundle/FrameworkBundle/Command/TranslationDebugCommand.php Outdated
@@ -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),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@fabpot
Copy link
Member

fabpot 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 debug_translation_add_strict_option branch from 0e4e925 to ed56b0d Compare March 6, 2019 13:18
@DAcodedBEAT
Copy link
Contributor Author

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 debug_translation_add_strict_option branch 2 times, most recently from e1b45e9 to 990dbc0 Compare May 28, 2019 16:55
@fabpot
Copy link
Member

fabpot 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 debug_translation_add_strict_option branch from 990dbc0 to 3a9f7c3 Compare July 11, 2019 11:18
@DAcodedBEAT DAcodedBEAT changed the title [FrameworkBundle][TranslationDebug] add strict option [FrameworkBundle][TranslationDebug] Return non-zero exit code on failure Jul 11, 2019
@DAcodedBEAT
Copy link
Contributor Author

@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 🙂

@fabpot
Copy link
Member

fabpot commented Jan 10, 2020

@DAcodedBEAT Still interested in finishing this PR?

@DAcodedBEAT DAcodedBEAT force-pushed the debug_translation_add_strict_option branch from 3a9f7c3 to 3dfcc8e Compare January 30, 2020 04:45
@DAcodedBEAT
Copy link
Contributor Author

Hello @fabpot! Sorry for the delay. Yes, I am still interested in finishing this Pull Request! I have pushed up the changes requested. Thank you for the reminder! 🙂

@DAcodedBEAT DAcodedBEAT requested a review from fabpot January 30, 2020 19:08
@DAcodedBEAT DAcodedBEAT force-pushed the debug_translation_add_strict_option branch from 3dfcc8e to e50ecfb Compare January 31, 2020 01:59
@fabpot
Copy link
Member

fabpot commented Jan 31, 2020

Thank you @DAcodedBEAT.

@fabpot fabpot mentioned this pull request Jan 31, 2020
Closed
@fabpot fabpot closed this Jan 31, 2020
@fabpot fabpot force-pushed the debug_translation_add_strict_option branch from e50ecfb to 0baafd8 Compare January 31, 2020 08:28
@fabpot fabpot closed this in 94efc95 Jan 31, 2020
@fabpot fabpot merged commit 0baafd8 into symfony:master Jan 31, 2020
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.1 May 4, 2020
@fabpot fabpot mentioned this pull request May 5, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@fabpot fabpot fabpot approved these changes

@Conan-Kudo Conan-Kudo Conan-Kudo approved these changes

Assignees
No one assigned
Labels
Feature FrameworkBundle Status: Reviewed
Projects
None yet
Milestone
5.1
Development

Successfully merging this pull request may close these issues.
5 participants
@DAcodedBEAT @nicolas-grekas @fabpot @Conan-Kudo @carsonbot