-
Notifications
You must be signed in to change notification settings - Fork 18
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
feat: atlas pull and push scripts | FC-55 #422
Conversation
Thanks for the pull request, @Amr-Nash! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
Note: CLA has been submitted but needs a day or two to reflect on GitHub. |
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.
Thanks @Amr-Nash, this pull request needs the following:
- squash the commits
- use conventional commits
- rename
make combine_translations
tomake extract_translations
The Extract will extract the English language translations from all modules to the I18N folder. The pull will pull all of the other languages translations from transifex to the I18N folder then split them to thier modules.
The Extract will extract the English language translations from all modules to the I18N folder. The pull will pull all of the other languages translations from transifex to the I18N folder then split them to thier modules.
5b86d24
to
94bc19a
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.
Thanks @Amr-Nash. I've reviewed the README and suggested many edits. Please adopt and revise and let me know when it's ready.
You'll be getting another review regarding the Python code from either me or @iamjazzar today.
@OmarIthawi Thank you for your feedback. It is much appreciated and I am waiting for the script review. |
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.
I've done an initial review, but I haven't tested the functionality yet. The code appears to be clean, but there are numerous nested for loops and some ambiguous names.
Any suggestion how to improve that? |
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.
Added some notes.
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.
Posting yesterday's review:
Hello @brian-smith-tcril , |
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.
Overall this is looking great! The code seems reasonable, so assuming it has been tested that part has a 👍 from me!
I left a couple suggestions in the README that I hope will make the instructions a little more clear.
Please let me know if you have any questions!
Thanks @brian-smith-tcril! Yes, the manual testing part is what we focused on. @Amr-Nash please address Brian notes and let's squash the code once more. |
Thanks @brian-smith-tcril for your feedback, |
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.
Thanks @Amr-Nash! I've tested it and it worked locally.
We need an iOS engineer to pull and compile, but overall I think it's ready for merge.
@volodymyr-chekyrta would you mind chekcing this pull request? We'd love a test compile from one of your team so we can continue with the other GitHub Actions pipeline steps. |
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.
Please add the language renaming as well.
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.
Tested ✅
Works fine for me 👍
README.md
Outdated
### Testing translations | ||
|
||
Until the [pull request #422](https://github.com/openedx/openedx-app-ios/pull/422) is merged, translations needs to be pulled from the testing branch `Zeit-Labs/openedx-translations` repo under `fc_55_sample` branch with the following options: | ||
``` bash | ||
make ATLAS_OPTIONS='--repository=Zeit-Labs/openedx-translations --revision=fc_55_sample' pull_translations | ||
``` |
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.
How about removing this section from the README?
It seems that the PR description is sufficient for testing
@Amr-Nash please replace the underscore for iOS. |
@OmarIthawi Done, please check. |
Extract will extract the English language resources from all modules to the I18N folder. Pull will pull all other languages translations from the openedx-traslations repository to the I18N folder then split them to thier modules.
4a4ecc7
to
0842d54
Compare
@Amr-Nash 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
Description
This pull request provides the CI scripts and developer tooling for the Atlas Translations Management Design
proposal related to OEP-58.
Testing instructions
Testing the
pull_translations
:When the above command is executed:
I18N/
directory.If the script is working as intended:
Testing the
extract_translations
:When the above command is executed:
en.lproj/Localizable.strings
files in all modules will be extracted"Module_name.OLD_KEY_of_the_entry"
.I18N/en.lproj/Localizable.strings
If this script is working as intended:
I18N/en.lproj/Localizable.strings
with the 3) corresponding comment, if present, placed before it.Status
We'll merge to this pull request into this branch multiple pull requests. We'll keep this open until we're ready for review.
This pull request is useful to watch progress.
Q and A
1. When are source strings extracted? what do the files look like?
I18N/en.lproj/Localizable.strings
and it will look like this:2. Does anything happen to those files before they are added to openedx-translations
The only extracted file is the English sources from the modules in the IOS repo as it is the only translation that lives in that repo.
TBC
3. When translation files are pulled via atlas what do the files look like?
When the translation files are pulled via atlas the files' structure is going to look like the structure below:
As an example the es.lproj/Localiztion.strings would look like:
I18N/es.lproj/Localizable.strings
4) What is done to those files after
atlas pull
to make them work in the app build process?After
atlas pull
is completed, all language translation files are retrieved, similar to the process described in the previous question. Subsequently, a Python script is executed to process each file. This script iterates through each translation file, splits it, removes the module name from the keys, and organizes each entry into its corresponding module.For instance, the
I18N/es.lproj/Localizable.strings
file is split into two separate files:Module_one/Module_one/es.lproj/Localizable.strings
Module_two/Module_two/es.lproj/Localizable.strings
Inside
Module_one/Module_one/es.lproj/Localizable.strings
, the content would resemble:Similarly, inside
Module_two/Module_two/es.lproj/Localizable.strings
, it would appear as:Once the Python script completes its task and the files are split, the make script proceeds to remove the entire
I18N
directory and its contents.5) Why is this needed?
This new process is being introduced to have the best combination of Developer Experience and Translator Experience.
The best experience for Translators requires combining source strings into as few transifex resources as possible. The best experience for Engineers requires splitting translation source files to fit within the modular architecture.
6) What should live in openedx translations?
Translations that were done by the open-edx translators are going to be stored in openedx translations.
7) What should live in atlas?
Atlas is the tool that we are using to pull the translations from openedx translations; therefor, no translations are stored in atlas.
8) What should live in the openedx-app-ios repo?
In the IOS app repo only the English sources are going to be stored.
TODO
python3-localizable
pip package for python3 compatibilityopenedx-atlas
in the virtual envmake extract_translations
Related issues