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

upload, build, download and update translations to/from Crowdin using js client api #11557

Merged
merged 41 commits into from
Nov 24, 2023

Conversation

krmanik
Copy link
Member

@krmanik krmanik commented Jun 6, 2022

Pull Request template

Purpose / Description

Move to new Crowdln JS Client API

Fixes

Fixes #8274

Approach

  1. Created a yarn project in tools/localization dir
mkdir ./tools/localization
cd ./tools/localization
mkdir src
yarn init
  1. Installed required dependencies
yarn add @crowdin/crowdin-api-client
  1. Created following files in src dir
    index.ts : For running functions defined below files
    upload.ts : For uploading English res/values files to Crowdin
    download.ts : For building/downloading English res/values files to Crowdin
    update.ts : For updating the files from extracted ankidroid.zip file

  2. Updated sync_translations.yml file

  3. Available commands, run from ./tools/localization
    upload : yarn start upload
    build and download : yarn start download
    extract ankidroid.zip : yarn start extract
    update : yarn start update

  4. The script in ts files are line by line conversion of python code to typescript

How Has This Been Tested?

It is tested on local repository. (Needs to add tests dir with test script and sample xml files)
Actions result
https://github.com/krmanik/Anki-Android-Copy/runs/6754662446?check_suite_focus=true

i18n_sync branch
https://github.com/krmanik/Anki-Android-Copy/compare/main...i18n_sync

(Change i18n_sync are all in English because not translated)

Learning (optional, can help others)

https://yarnpkg.com/cli/install
https://www.typescriptlang.org/docs/
https://github.com/crowdin/crowdin-api-client-js
https://support.crowdin.com/api/v2/#section/Introduction/Crowdin-API-Clients

Links to blog posts, patterns, libraries or addons used to solve this problem

Checklist

Please, go through these checks before submitting the PR.

  • You have not changed whitespace unnecessarily (it makes diffs hard to read)
  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • Your code follows the style of the project (e.g. never omit braces in if statements)
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code
  • UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
  • UI Changes: You have tested your change using the Google Accessibility Scanner

.github/workflows/sync_translations.yml Outdated Show resolved Hide resolved
tools/localization/src/update.ts Outdated Show resolved Hide resolved
Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

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

Looks great! Couple of questions

tools/localization/src/update.ts Outdated Show resolved Hide resolved
@david-allison david-allison added the Review High Priority Request for high priority review label Jun 6, 2022
@Arthur-Milchior
Copy link
Member

I'm looking at the API. It seems, if I understand correctly, that there is now a way to interact with screenshot too. If so, do you think you could, in another PR, maybe have a way to:

  • download all screenshot (right now, I can't find a way to make a back-up. Which means if crowd-in stop, we lose hours of work)
  • upload screenshot from PR; which will hopefully win a lot of time and avoid forgetting to do the work.

I'm still reviewing.

@david-allison
Copy link
Member

david-allison commented Jun 7, 2022

We do have a way to download screenshots, let's move this to: #8621.

I feel uploading screenshots is overengineering at this point - far too much that can go wrong with it for it to be useful

Copy link
Member

@Arthur-Milchior Arthur-Milchior left a comment

Choose a reason for hiding this comment

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

Thanks for the change. Clearly, it seems we are late to leave V1, so that's really cool you work on it. Thanks also because this lead me to learn things while reviewing it. I'm not that used to JS to be honest. So having to open JS file in intellij tool, and check the API description from crowdin is a new experience that is nice to have

I reviewed up to "fix regex, string replacement" and then I discovered that some of the questions from early commits were already answered in later one.
It seems like still there are some suggestions and question that are still relevant now.
I'll do another pass when I get feedback on those first questions and remarks, so that I can use your experience to improve my review
I thought it was atomic commits, as often here. Is it instead supposed to be a single squashed commit where everything should be reviewed together?

tools/localization/yarn.lock Show resolved Hide resolved
tools/localization/src/upload.ts Outdated Show resolved Hide resolved
tools/localization/src/upload.ts Outdated Show resolved Hide resolved
tools/localization/src/upload.ts Outdated Show resolved Hide resolved
tools/localization/src/upload.ts Outdated Show resolved Hide resolved
tools/localization/src/update.ts Outdated Show resolved Hide resolved
tools/localization/src/update.ts Outdated Show resolved Hide resolved
tools/localization/src/update.ts Outdated Show resolved Hide resolved
tools/localization/src/update.ts Show resolved Hide resolved
tools/localization/src/update.ts Outdated Show resolved Hide resolved
@krmanik
Copy link
Member Author

krmanik commented Jun 8, 2022

I went through each review and updated it. If I missed any review let me know.

@krmanik
Copy link
Member Author

krmanik commented Jun 8, 2022

Getting url undefined errors, may be 30 minutes wait.

$ node ./dist/index.js download
downloading...
Building ZIP on server...
Built.
Downloading Crowdin file
node:internal/errors:465
    ErrorCaptureStackTrace(err);
    ^
TypeError [ERR_INVALID_ARG_TYPE]: The "url" argument must be of type string. Received undefined
    at new NodeError (node:internal/errors:372:5)
    at validateString (node:internal/validators:120:11)
    at Url.parse (node:url:168:3)
    at Object.urlParse [as parse] (node:url:155:13)
    at dispatchHttpRequest (/home/runner/work/Anki-Android-Copy/Anki-Android-Copy/tools/localization/node_modules/axios/lib/adapters/http.js:133:22)
    at new Promise (<anonymous>)
    at httpAdapter (/home/runner/work/Anki-Android-Copy/Anki-Android-Copy/tools/localization/node_modules/axios/lib/adapters/http.js:4[9](https://github.com/krmanik/Anki-Android-Copy/runs/6791577721?check_suite_focus=true#step:9:10):[10](https://github.com/krmanik/Anki-Android-Copy/runs/6791577721?check_suite_focus=true#step:9:11))
    at dispatchRequest (/home/runner/work/Anki-Android-Copy/Anki-Android-Copy/tools/localization/node_modules/axios/lib/core/dispatchRequest.js:58:10)
    at Axios.request (/home/runner/work/Anki-Android-Copy/Anki-Android-Copy/tools/localization/node_modules/axios/lib/core/Axios.js:109:[15](https://github.com/krmanik/Anki-Android-Copy/runs/6791577721?check_suite_focus=true#step:9:16))
    at wrap (/home/runner/work/Anki-Android-Copy/Anki-Android-Copy/tools/localization/node_modules/axios/lib/helpers/bind.js:9:15) {
  code: 'ERR_INVALID_ARG_TYPE'
}
error Command failed with exit code 1.

Copy link
Member

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

I feel like all of my comments were either sort of "structural" or minor, with the exception of the "maybe ESM works?" request for an attempt

In general this looks pretty close to good, I'm excited to get it merged and have the translations stuff in a language I prefer more and which will share skillsets with "modern" Anki (typescript) vs python which I am always a bit lost in...

tools/localization/src/constants.ts Outdated Show resolved Hide resolved
tools/localization/src/constants.ts Outdated Show resolved Hide resolved
tools/localization/src/constants.ts Show resolved Hide resolved
tools/localization/src/index.ts Outdated Show resolved Hide resolved
tools/localization/src/index.ts Outdated Show resolved Hide resolved
.github/workflows/sync_translations.yml Show resolved Hide resolved
tools/localization/src/download.ts Outdated Show resolved Hide resolved
tools/localization/src/update.ts Outdated Show resolved Hide resolved
tools/localization/src/update.ts Outdated Show resolved Hide resolved
tools/localization/package.json Outdated Show resolved Hide resolved
@mikehardy mikehardy added Needs Author Reply Waiting for a reply from the original author Dev Development, testing & CI labels Jun 25, 2022
@krmanik

This comment was marked as outdated.

@mikehardy

This comment was marked as outdated.

mikehardy

This comment was marked as resolved.

@krmanik

This comment was marked as outdated.

@krmanik

This comment was marked as outdated.

@krmanik

This comment was marked as outdated.

@david-allison

This comment was marked as outdated.

these do not need to be copied from the zip to the app res directories
appending without clearing it out means it will grow infinitely
this was changed in python while new system was in development,
synchronizing change here
@mikehardy mikehardy added Review High Priority Request for high priority review squash-merge The pull request currently requires maintainers to "Squash Merge" and removed Needs Author Reply Waiting for a reply from the original author labels Nov 23, 2023
Copy link
Member

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

This is ready to go pending #14801 - it's a workflow that we can only (or should only...) run on main so post-merge it will need a few runs to make sure the yaml looks good, and careful monitoring for the first few string update/deletes, but I think it's good to go

ran this a bunch locally while testing, fixed every little thing I saw during execution

re-read the manage-crowdin and update-localizations python (which I've worked with a bunch in the past) and compared it to everything in here while working here - I think it all checks out and to help future explorers (or future me) I added comments for anything I had to think about at all

@mikehardy
Copy link
Member

I'm busy rest of the day today but will merge this tomorrow morning my time (about 16hrs from now?) assuming no one sees anything dire

Then I can do the first few runs with it and make sure it is well behaved. Strings are up to date so we'll have small diffs to start...

Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

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

Nothing blocking, a few nits for optional follow-ups

tools/localization/package.json Show resolved Hide resolved
tools/localization/src/constants.ts Show resolved Hide resolved
tools/localization/src/download.ts Outdated Show resolved Hide resolved
tools/localization/src/download.ts Show resolved Hide resolved
tools/localization/src/update.ts Outdated Show resolved Hide resolved
tools/localization/src/update.ts Outdated Show resolved Hide resolved
@krmanik
Copy link
Member Author

krmanik commented Nov 24, 2023

I have tested it locally and it is working as expected. I have added Readme file for the commands.

@mikehardy
Copy link
Member

Fantastic, thanks again @krmanik - I'll do what I can on David's suggestion then merge this and do some careful runs of the i18n sync afterwards. There is always some tiny thing so I won't be surprised if there is some little thing on the yaml changes in the workflow but this is fantastic and I'm excited to get this in

- use numerical separator
- update comment about language tags (2- vs 3-letter etc)
- typos and formatting
- mark package as non-publishable ("private")
Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

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

Just to confirm: it was good to go before and is still good to go

@mikehardy
Copy link
Member

Just to confirm: it was good to go before and is still good to go

prompted great additions though, much appreciated

@mikehardy mikehardy merged commit ad94cd6 into ankidroid:main Nov 24, 2023
7 checks passed
@github-actions github-actions bot added this to the 2.17 release milestone Nov 24, 2023
@github-actions github-actions bot removed Review High Priority Request for high priority review Keep Open avoids the stale bot squash-merge The pull request currently requires maintainers to "Squash Merge" labels Nov 24, 2023
@mikehardy
Copy link
Member

mikehardy commented Nov 24, 2023

Okay - now bear with me while I do the first couple runs.

I know that it will do some html-entitification on the first run as I've seen it locally (but not committed those changes to the language files, saving it for the run...) and I've verified it is actually what we want, so it has a nice first test case

I'm probably going to YOLO fixes direct on main to the YAML though until I get it working. I can't risk having the cloud state of crowdin (which is unbranchable) not tracking the local state of workflow runs here. Not my favorite thing to do to walk on a tightrope without a net but I believe it will work out...

@krmanik krmanik deleted the crowdin-update-sync branch November 24, 2023 14:33
@mikehardy mikehardy mentioned this pull request Nov 24, 2023
5 tasks
Copy link
Contributor

Hi there @krmanik! This is the OpenCollective Notice for PRs merged from 2023-11-01 through 2023-11-30

If you are interested in compensation for this work, the process with details is here:

https://github.com/ankidroid/Anki-Android/wiki/OpenCollective-Payment-Process#how-to-get-paid

We only post one comment per person per month to avoid spamming you, regardless of the number of PRs merged, but this note applies to all PRs merged for this month

Please understand that our monthly budget is never guaranteed to cover all claims - the cap on payments-per-person may be lower, but we try to make our process as fair and transparent as possible, we just need your understanding.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Dev Development, testing & CI Priority-High
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Deprecation] Move to new Crowdln API
6 participants