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

[Translation] Add lint:translations command #57101

Merged
merged 1 commit into from
May 31, 2024

Conversation

Kocal
Copy link
Contributor

@Kocal Kocal commented May 24, 2024

Q A
Branch? 7.2
Bug fix? no
New feature? yes
Deprecations? no
Issues Fix #...
License MIT

Hi everyone (cc @welcoMattic),

This PR adds a new command lint:translations in the Translator component, which lints the translations like lint: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 }, missing other in plural, 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 the Console 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 :)

@Kocal Kocal requested a review from welcoMattic as a code owner May 24, 2024 12:01
@carsonbot carsonbot added this to the 7.2 milestone May 24, 2024
@carsonbot carsonbot changed the title [Translator] Add lint:translations command [Translation] Add lint:translations command May 24, 2024
@Kocal Kocal force-pushed the feat/lint-translations branch 4 times, most recently from 4b07004 to 85f3094 Compare May 24, 2024 12:46
@Kocal
Copy link
Contributor Author

Kocal commented May 24, 2024

Fabbot has some issues with the license header in tests files, if I keep it then Coding Standard fails, but if I remove it then License Headers fails. :/

EDIT: ok that's fine now, I guess that was due to the declare(strict_types=1) that I've removed.

@Kocal Kocal force-pushed the feat/lint-translations branch 7 times, most recently from a6e3933 to 9d8c87a Compare May 24, 2024 17:08
@Kocal Kocal force-pushed the feat/lint-translations branch 5 times, most recently from 89ef1a6 to 8f1bde8 Compare May 28, 2024 05:23
@Zombaya
Copy link

Zombaya commented May 29, 2024

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 🙂

@Kocal Kocal force-pushed the feat/lint-translations branch 2 times, most recently from fb43300 to f4ec900 Compare May 29, 2024 14:12
Copy link
Member

@fabpot fabpot left a 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.

@fabpot
Copy link
Member

fabpot commented May 31, 2024

Thank you @Kocal.

@fabpot fabpot merged commit f6a5cbd into symfony:7.2 May 31, 2024
8 of 10 checks passed
@Kocal Kocal deleted the feat/lint-translations branch May 31, 2024 05:50
{
$this
->setDefinition([
new InputOption('locales', null, InputOption::VALUE_OPTIONAL | InputOption::VALUE_IS_ARRAY, 'Specify the locales to lint.', $this->enabledLocales),
Copy link
Member

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)

fabpot added a commit that referenced this pull request Jul 5, 2024
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants