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

docs: design for atlas pull and translations management | FC-0055 #367

Merged
merged 1 commit into from
Mar 29, 2024

Conversation

OmarIthawi
Copy link
Member

@OmarIthawi OmarIthawi commented Mar 25, 2024

Please review this proposal. We're set to start on April 1st, 2024.

Please let us know if you have any questions or objections.

I've omitted many OEP-58 related details, to keep the discussion focused on the mobile parts.

Preview link the proposal doc:

This pull request is part of the FC-0055 project which implements the Translation Infrastructure update OEP-58 for mobile apps.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Mar 25, 2024
@openedx-webhooks
Copy link

Thanks for the pull request, @OmarIthawi! 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:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

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.

@OmarIthawi
Copy link
Member Author

@miankhalid @volodymyr-chekyrta I've assigned you as reviewers, but please feel free to re-assign to someone else as you see fit.

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated

pull_translations:
make clean_translations_temp_directory
atlas pull $(ATLAS_OPTIONS) translations/openedx-app-ios/I18N/I18N/:I18N/I18N/
Copy link
Member Author

Choose a reason for hiding this comment

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

The atlas pull command represents a real command which will work once we implement the combine_translations step.

@OmarIthawi OmarIthawi changed the title docs: design for atlas pull docs: design for atlas pull and translations management Mar 27, 2024
Copy link

@brian-smith-tcril brian-smith-tcril left a comment

Choose a reason for hiding this comment

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

I did an initial pass. Overall this is looking quite good! I left a few comments:

  • Phrasing suggestions
  • Question about the use of the term "clean architecture app" - it feels a little confusing in this context since we also refer to each mobile app as an "app"
  • Question about ways to handle identical string keys (namespacing?)

docs/0001-strategy-for-maintaining-OS-versions.rst Outdated Show resolved Hide resolved
docs/0001-strategy-for-maintaining-OS-versions.rst Outdated Show resolved Hide resolved
docs/0001-strategy-for-maintaining-OS-versions.rst Outdated Show resolved Hide resolved
docs/0002-atlas-translations-management.rst Outdated Show resolved Hide resolved
docs/0002-atlas-translations-management.rst Outdated Show resolved Hide resolved
docs/0002-atlas-translations-management.rst Outdated Show resolved Hide resolved
docs/0002-atlas-translations-management.rst Outdated Show resolved Hide resolved
docs/0002-atlas-translations-management.rst Outdated Show resolved Hide resolved
docs/0002-atlas-translations-management.rst Outdated Show resolved Hide resolved
docs/0002-atlas-translations-management.rst Outdated Show resolved Hide resolved
@OmarIthawi
Copy link
Member Author

@volodymyr-chekyrta @brian-smith-tcril I've updated the proposal with the prefix option, and addressed all the other comments.

Please let me know if there's anything else.

@OmarIthawi OmarIthawi changed the title docs: design for atlas pull and translations management docs: design for atlas pull and translations management | FC-0055 Mar 27, 2024
Copy link
Contributor

@volodymyr-chekyrta volodymyr-chekyrta left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@OmarIthawi OmarIthawi force-pushed the atlas branch 2 times, most recently from 961cbb3 to de0329b Compare March 27, 2024 12:43
@OmarIthawi
Copy link
Member Author

Thanks a lot @brian-smith-tcril and @volodymyr-chekyrta for the reviewing this so quick :)

I'm happy about the outcome, and we'll start working on it as soon as possible.

I've removed the Makefile since it was added without scripts so this pull request becomes mergeable:

For reference, here was the Makefile contents:

openedx-app-ios $ cat Makefile 
clean_translations_temp_directory:
	rm -rf I18N/I18N/


pull_translations:
	make clean_translations_temp_directory
	atlas pull $(ATLAS_OPTIONS) translations/openedx-app-ios/I18N/I18N/:I18N/I18N/
	python scripts/split_translation_files.py
	make clean_translations_temp_directory


combine_translations:
	make clean_translations_temp_directory
	python scripts/combine_translations_files.py

We'll add it back once we implement the referenced scripts.

@volodymyr-chekyrta
Copy link
Contributor

@OmarIthawi, any concerns about switching the PR target branch to the develop?
We follow feature -> develop -> main git flow

Copy link

@brian-smith-tcril brian-smith-tcril left a comment

Choose a reason for hiding this comment

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

I'm super happy with the direction this is heading! I left a couple more comments about wording, and one with a question about message descriptions.

docs/0002-atlas-translations-management.rst Outdated Show resolved Hide resolved
docs/0002-atlas-translations-management.rst Outdated Show resolved Hide resolved
docs/0002-atlas-translations-management.rst Outdated Show resolved Hide resolved
Comment on lines 35 to 47
Decision
********

This decision requires a few changes to the mobile app translation workflow:

- Translation files are no longer comitted directly to the mobie app repositories (`openedx-app-ios`_ / `openedx-app-android`_)
- Keep the i18n string files split into each module without hurting Developer Experience.
- String files are combined into a single file before uploading it to Transifex
(through the `openedx-translations repository`_)
- Translators can translate the entire app within a single transifex resource
- Before releasing to App Store, developers need to run a new ``make pull_translations`` command
- After translations are pulled the translation files are split into each module
as if it has been individual resource (a new script)

Choose a reason for hiding this comment

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

This section is a little confusing. Some bullet points are describing changes to the workflow, some are describing ways in which the workflow will not change, and some are just describing the proposed new workflow without reference to anything changing/staying the same.

It might be helpful to split this up into a couple subsections:

New Workflow
* Bullets
* Describing
* New
* Workflow
Notable changes
* Bullets describing notable ways in which the new workflow is different from the previous workflow

Or maybe it could all be done in one section focusing on the new workflow, with sub-bullets under any bullets that include notable changes.

docs/0002-atlas-translations-management.rst Outdated Show resolved Hide resolved
docs/0002-atlas-translations-management.rst Show resolved Hide resolved
@OmarIthawi OmarIthawi changed the base branch from main to develop March 27, 2024 19:40
@OmarIthawi OmarIthawi dismissed volodymyr-chekyrta’s stale review March 27, 2024 19:40

The base branch was changed.

Copy link

@brian-smith-tcril brian-smith-tcril left a comment

Choose a reason for hiding this comment

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

I think these are the last of my comments. This is almost ready to merge!

docs/0002-atlas-translations-management.rst Outdated Show resolved Hide resolved
docs/0002-atlas-translations-management.rst Outdated Show resolved Hide resolved
docs/0002-atlas-translations-management.rst Outdated Show resolved Hide resolved
docs/0002-atlas-translations-management.rst Outdated Show resolved Hide resolved
@OmarIthawi
Copy link
Member Author

@brian-smith-tcril I've added a detailed step-by-step workflow and a new notable changes section.

Please double check and let me know if you have any notes.

Copy link

@brian-smith-tcril brian-smith-tcril left a comment

Choose a reason for hiding this comment

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

I'm not sure if these suggestions will show up or not

docs/0002-atlas-translations-management.rst Outdated Show resolved Hide resolved
docs/0002-atlas-translations-management.rst Outdated Show resolved Hide resolved
docs/0002-atlas-translations-management.rst Outdated Show resolved Hide resolved
docs/0002-atlas-translations-management.rst Outdated Show resolved Hide resolved
docs/0002-atlas-translations-management.rst Outdated Show resolved Hide resolved
docs/0002-atlas-translations-management.rst Outdated Show resolved Hide resolved
docs/0002-atlas-translations-management.rst Outdated Show resolved Hide resolved
@OmarIthawi
Copy link
Member Author

OmarIthawi commented Mar 27, 2024

@brian-smith-tcril all minor changes are applied. Good catches I should say :)

I think docs proof-reading has a legit AI use case the GitHub needs! I miss such feature.

@brian-smith-tcril
Copy link

@brian-smith-tcril all minor changes are applied. Good catches I should say :)

I think docs proof-reading has a legit AI use case the GitHub needs! I miss such feature.

I've used the free version of grammarly when writing OEPs/ADRs before and it's been helpful

@brian-smith-tcril
Copy link

Thanks for putting up with all the back and forth during the review process! I left one tiny last comment but this looks good to go. Approved 👍

@OmarIthawi
Copy link
Member Author

I've used the free version of grammarly when writing OEPs/ADRs before and it's been helpful

I'll try that! Writing RST requires a lot of effort which definitly need some tooling help esp. an ESL person.

Thanks for putting up with all the back and forth during the review process! I left one tiny last comment but this looks good to go. Approved 👍

Thanks for all the reviews! It's sometimes hard to articulate ideas and follow up with edit notes, but it makes it much clearer for future reviewers.

@brian-smith-tcril
Copy link

I'll try that! Writing RST requires a lot of effort which definitly need some tooling help esp. an ESL person.

For sure! It's hard enough for me with English as a first language!

@OmarIthawi OmarIthawi requested review from brian-smith-tcril and volodymyr-chekyrta and removed request for brian-smith-tcril March 28, 2024 13:50
@OmarIthawi
Copy link
Member Author

Thanks @volodymyr-chekyrta and @brian-smith-tcril for quickly triaging this decision. Both of you have already approved it. Kindly merge.

We'll start working in the coming few days.

@volodymyr-chekyrta volodymyr-chekyrta merged commit 47a9757 into openedx:develop Mar 29, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants