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

ci(actions): Update translations automatically #13773

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Holzhaus
Copy link
Member

@Holzhaus Holzhaus commented Oct 15, 2024

This automatically fetches translations from transifex, compiles them and then opens a PR to merge them. See Holzhaus#25 for an example PR that was opened by the bot.

TODO:

  • Agree on a reasonable cron date (or stick with workflow dispatch)
  • Add a TRANSIFEX_TOKEN secret to the repo secrets.

with:
branch: pull-translations/${{ matrix.branch }}
commit-message: |-
Pull Transifex translations for ${{ matrix.branch }}
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
Pull Transifex translations for ${{ matrix.branch }}
chore(translations): Pull ${{ matrix.branch }} branch from Transifex

Copy link
Member

Choose a reason for hiding this comment

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

super nit-y, but commitlint enforce the subject case to start with lower case by default. I know we don't use this linter and that subject-case is highly controversial,s o happy to keep it as is, but I think it's common to use lower cased subject

Suggested change
Pull Transifex translations for ${{ matrix.branch }}
chore(translations): pull ${{ matrix.branch }} branch from Transifex

Copy link
Member Author

Choose a reason for hiding this comment

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

Are you

super nit-y, but commitlint enforce the subject case to start with lower case by default.

Are you sure about that? To me it looks like Start Case, Sentence case, lower case and UPPER CASE are allowed by default: https://commitlint.js.org/reference/rules.html#subject-case
The actual Conventional Commit spec does not enforce any specific case for the subject: https://www.conventionalcommits.org/en/v1.0.0/#are-the-types-in-the-commit-title-uppercase-or-lowercase

Personally I'm in favor of Sentence case for readability (and also because all lowercase looks kind of lazy to me xD).


on:
schedule:
- cron: "0 0 1 * *"
Copy link
Member Author

Choose a reason for hiding this comment

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

This will run on the first day of every month at 12 a.m.
Is is too often? Or too rare? Suggestions welcome.

Copy link
Member

Choose a reason for hiding this comment

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

I would almost vote too rare - how expansive is a run? (build time, bandwidth, ...)

Copy link
Member Author

Choose a reason for hiding this comment

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

@JoergAtGithub
Copy link
Member

Do I see it correct, that compilation of all .ts files together need only 2 seconds CI time? If yes I would suggest to move this step in the normal build and get rid of the binary files stored in the git repository.

@JoergAtGithub
Copy link
Member

For a PR review, neither the .qm files(binary) nor the .ts files (huge because of line number changes) are suitable.
Maybe we should generate a simple text file with just 2 columns out of each .ts file. One for the English string, and one for the translation. Than the diff would show us translation changes, and not line number changes.

@Holzhaus
Copy link
Member Author

Do I see it correct, that compilation of all .ts files together need only 2 seconds CI time? If yes I would suggest to move this step in the normal build and get rid of the binary files stored in the git repository.

"Two stupid people, one thought" as we say in Germany 😛

https://mixxx.zulipchat.com/#narrow/stream/109171-development/topic/Handling.20of.20Translations/near/477077262

@daschuer
Copy link
Member

I don't think that we can/will ever use this PR for review translations. The review needs to be done at Transifex by native speakers. Let's just rely on Transifex here and merge this blindly.
You may argue that we may spot destructive mass edits. But who wants to give a LGTM finally, without knowing what can be offensive?

@Holzhaus
Copy link
Member Author

@daschuer Agreed, I did not open this PR to ensure manual reviews of translation PRs, just to make pulling translations easier and less work, and maybe we can do it more often.

@acolombier
Copy link
Member

acolombier commented Oct 16, 2024

Let's just rely on Transifex here and merge this blindly.

I think it would be great to still have PR, even if we don't review them with as much involvement as we would do for code. We had a case a few month ago where Deejaygirl someone managed to sneak some insult in the Mixxx code base. Having a PR with a quick diff could be an easier way to spot those

One question on PR - does it update the previously open PR or does it create a new one? I know renovate does that with deps update so I'm wondering if this is an option to reduce spam.

@Holzhaus
Copy link
Member Author

One question on PR - does it update the previously open PR or does it create a new one? I know renovate does that with deps update so I'm wondering if this is an option to reduce spam.

It does update the original PR. See https://github.com/peter-evans/create-pull-request?tab=readme-ov-file#action-behaviour.

@Holzhaus
Copy link
Member Author

Having a PR with a quick diff could be an easier way to spot those

image

The diff is huge. However, we could do an automated scan using something like https://github.com/IEvangelist/profanity-filter. I'm also currently looking into allowlisting certain PRs to be auto-merged by CI once all checks passed.

@acolombier
Copy link
Member

Yeah I realised that it was regenerating a lot of stuff after commenting.
But a CI job with profanity filter+auto merge sounds like the way to go.

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.

4 participants