-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Translation] Add lint:translations
command
#57101
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
Conversation
lint:translations
commandlint:translations
command
4b07004
to
85f3094
Compare
Fabbot has some issues with the license header in tests files, if I keep it then EDIT: ok that's fine now, I guess that was due to the |
a6e3933
to
9d8c87a
Compare
src/Symfony/Component/Translation/Command/TranslationLintCommand.php
Outdated
Show resolved
Hide resolved
89ef1a6
to
8f1bde8
Compare
As a symfony-user, not a symfony-contributor, this looks very useful to me and something we would like to use ourselves. There can never be too many lint-commands to be used in CI 🙂 |
src/Symfony/Component/Translation/Command/TranslationLintCommand.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Translation/Command/TranslationLintCommand.php
Outdated
Show resolved
Hide resolved
fb43300
to
f4ec900
Compare
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.
LGTM, but with a few minor comments.
src/Symfony/Component/Translation/Command/TranslationLintCommand.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Translation/Command/TranslationLintCommand.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Translation/Command/TranslationLintCommand.php
Outdated
Show resolved
Hide resolved
f4ec900
to
e22aa1c
Compare
Thank you @Kocal. |
{ | ||
$this | ||
->setDefinition([ | ||
new InputOption('locales', null, InputOption::VALUE_OPTIONAL | InputOption::VALUE_IS_ARRAY, 'Specify the locales to lint.', $this->enabledLocales), |
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.
InputOption::VALUE_OPTIONAL
does not make sense here IMO. This would allow passing the option as bin/console lint:translations --locales
, without passing a value for it.
Making the value of an option optional is a rare case and is not what you want in most commands.
Also, I think locale
would be a better name for the option as lint:translations --locale fr --locale en
makes more sense than lint:translations --locales fr --locales en
(InputOption::VALUE_IS_ARRAY
works by repeating the option in the command, but each occurrence provides a single value)
…nslations (xabbuh) This PR was merged into the 7.2 branch. Discussion ---------- [Translation] enhance the locale handling when linting translations | Q | A | ------------- | --- | Branch? | 7.2 | Bug fix? | no | New feature? | no | Deprecations? | no | Issues | Fix #57101 (comment) | License | MIT Commits ------- 27e9faa enhance the locale handling when linting translations
Hi everyone (cc @welcoMattic),
This PR adds a new command
lint:translations
in the Translator component, which lints the translations likelint:yaml
could lint Yaml files,lint:templates
lints Twig files, etc...Why?
Our application uses translations from Lokalise. They are contributed by non-tech users and sometimes they can contains issues (missing
}
, missingother
inplural
, etc...).Thoses issues results in a error 500 because the ICU translations can not be correctly parsed, and we want to prevent this by linting the translations before deploying (if the command fails, we don't deploy, and we don't have errors 500).
The current approach is a bit naive, at the moment it simply call
TranslatorInterface#trans()
and catch exceptions (if any), but it can be improved in the future.PS: During the time between creating the command and opening the PR, we used it on our app and we were able to detect 5/6 invalid translations :)
WDYT? Thanks :)
Also, quick question, how do Symfony/PHP developers deals with
trim_trailing_whitespace = true
from the.editorconfig
, and asserting on theConsole
output (which contains trailing spaces)?Do they disable EditorConfig in their IDE? Thanks!
EDIT: I don't have the issue anymore by right-trailing white chars for each lines, but I'm still curious :)