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

chore: Sync translations #4136

Merged
merged 1 commit into from
Dec 16, 2024
Merged

chore: Sync translations #4136

merged 1 commit into from
Dec 16, 2024

Conversation

github-actions[bot]
Copy link
Contributor

Sync translations from crowdin.com/project/revanced

@LisoUseInAIKyrios
Copy link
Contributor

It's not clear why, but all the exported translations are now empty.

No config was recently changed, so this is odd.

@LisoUseInAIKyrios
Copy link
Contributor

Somehow all the patch strings were deleted by Crowdin.

@LisoUseInAIKyrios
Copy link
Contributor

It's really annoying there is no rollback function for the free Crowdin plan.

It makes glitches like this a pain. All the translations made in the last 2 days must be manually added again.

@LisoUseInAIKyrios
Copy link
Contributor

Because the string history is not available with a free plan, it's not clear where it went wrong.

But I think it was the regex preprocessor not running or failing for some reason.

If anything like this happens again, preprocessing can be moved to the GitHub side so it won't ever upload corrupt or incorrectly modified strings.

@github-actions github-actions bot force-pushed the feat/translations branch 2 times, most recently from 0847eac to 5ffac9d Compare December 16, 2024 10:11
@LisoUseInAIKyrios
Copy link
Contributor

@oSumAtrIX I think the Crowdin action should be split apart into two actions.

The first action only syncs to the translation branch (and not create a pull request), but it's scheduled to run every day (or possibly more than once a day).

And then a second action would be to create a Crowdin PR.

Then, the translation history and changes are available here on GitHub which solves the Crowdin free plan not giving access to history or reverts.

@github-actions github-actions bot force-pushed the feat/translations branch 2 times, most recently from 9b4c82b to 5505364 Compare December 16, 2024 10:31
@Ushie
Copy link
Member

Ushie commented Dec 16, 2024

Crowdin can probably be configured to not force push each sync and rather just push the new changes

@LisoUseInAIKyrios
Copy link
Contributor

Yes that would be helpful.

@Ushie
Copy link
Member

Ushie commented Dec 16, 2024

I believe you can just change this to the Crowdin branch
https://github.com/ReVanced/revanced-patches/blob/main/.github/workflows/pull_strings.yml#L18

@oSumAtrIX
Copy link
Member

@oSumAtrIX I think the Crowdin action should be split apart into two actions.

The first action only syncs to the translation branch (and not create a pull request), but it's scheduled to run every day (or possibly more than once a day).

And then a second action would be to create a Crowdin PR.

Then, the translation history and changes are available here on GitHub which solves the Crowdin free plan not giving access to history or reverts.

Why would we not want it to create a PR? The PR action in the workflow does not create a new PR if the PR already exists, on first sync the PR will be openened, and on future syncs the PR will remain open to be merged at any time. The "translation history" would be present regardless of the PR. So we only need to schedule a run of the workflow every day.

@LisoUseInAIKyrios
Copy link
Contributor

The only reason to sync without opening a PR, is when pushing to a branch with a PR then everyone watching this repo will get a daily email on every sync.

If the sync is done to a branch with no open PR, then no email spam will be sent. Plus it could run the task every 6 hours or whatever so if Crowdin goes bonkers less translation work is lost since the most recent GitHub artifacts to restore are newer.

@oSumAtrIX
Copy link
Member

You can disable notifications but also filter for the PR title in the email when receiving a mail from GitHub.

image

Splitting the workflow into two is not elegant. We could make the step that opens a PR only trigger on dispatch or in longer schedules, such as every week. Or completely get rid of the step and open PRs manually.

@LisoUseInAIKyrios
Copy link
Contributor

LisoUseInAIKyrios commented Dec 16, 2024

Removing the PR part of the pull strings action is ok.

But there could be a really simple Strings pull request action to open a PR with everything prefilled (PR title, merge to dev, etc), and save a few steps and reduce mistakes. It would not sync anything just save steps when merging.

@oSumAtrIX
Copy link
Member

oSumAtrIX commented Dec 16, 2024

In that case the workflow should include it as a step that is triggered on workflow dispatch. However, at this point, the workflow can also just merge the branch in the first place (A PR would still be good as a standard review process)

@oSumAtrIX oSumAtrIX merged commit 0b2a10f into dev Dec 16, 2024
@LisoUseInAIKyrios
Copy link
Contributor

a PR is definitely still good to have since the AI translations can go off the rails and start adding html and sometimes error messages are interpreted as translations.

It's the only time a spot check works and really is needed, since no understanding of the languages is needed to see the problems.

@LisoUseInAIKyrios
Copy link
Contributor

Some of the spoof strings are outdated, but they'll get updated next time.

@LisoUseInAIKyrios
Copy link
Contributor

Can we change the crowdin actions now?

It was a huge pain restoring Crowdin to it's prior state (and it's still missing some translations). If this happens again the cost needs to be minimized.

@oSumAtrIX
Copy link
Member

Yes. On the PR step you can add an if guard that checks, if the workflow was triggered by a workflow dispatch event.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants