-
Notifications
You must be signed in to change notification settings - Fork 53
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: allow pulling translations by module name rather than repo name #353
Conversation
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:
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. |
24ffa49
to
f4292a8
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.
Overall this feels like a good path forward. I left a small comment about a place where the documentation doesn't feel fully clear to me.
python-modules/README.rst
Outdated
@@ -0,0 +1,11 @@ | |||
Python Modules Links |
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.
It's not fully clear to me how this is working. I understand how the links are being created in extract-translation-source-files
, and I understand that we need to be able to look things up by module name instead of repo name, but it's not clear to me how this works on the edx-platform
side.
Is the plan to update edx-platform
to look specifically for a python-modules
directory as part of the sparse checkout via atlas?
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.
@brian-smith-tcril yes, this has been tested locally with the edx-platform.
The following is the management command that is responsible about pulling the XBlock translations by module name:
We'll share a demo soon to clarify the design we're targeting, but for this PR I think it's ready and it's ready to merge after once you find that the documentation is clear for the purpose of this repository.
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 think the biggest thing I've having trouble with is the name of the directory. If we look at the root of openedx-translations
now, it's fairly clear what every directory is for:
docs
requirements
scripts
translations
Each directory name clearly implies what the directory is for and what it does. It is not clear from reading python-modules
what that directory is for, what it does, and why it lives in the root of the repo.
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.
@brian-smith-tcril yes, sure the name makes more sense now.
I'm wondering about edx-platform-translations
? It sounds even more specific, but I'm unaware of the long term plan for the edx-platform
repo name though, is there an intention to rename it to openedx-platform
?
Anyway, both the directory name and the README.rst file has been updated.
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.
What if they lived in translations/edx-platform-links/
? I think having translations
and openedx-translations
as directories at the top level is confusing.
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.
translations/edx-platform-links/
That should be less confusing. Thanks!
8491d3d
to
f597821
Compare
8778c3d
to
e13d740
Compare
@brian-smith-tcril rename done. Also tested on our fork:
|
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.
This looks great! 1 little comment about what appears to be a typo in the readme, other than that this should be good to go!
Pulling ``edx-platform`` translations with Atlas | ||
------------------------------------------------ | ||
|
||
The ``atlass`` command has been integrated in the ``edx-platform`` repository therefore there's no need to run ``atlas pull`` manually. Regardless, this section shows the manual command arguments because it's needed for some advanced usage of the edX Platform such as installing custom XBlocks that's not part of the openedx github organizaiton. |
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.
is atlass
here (with the double s) a typo?
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.
yup, and fixed
…po name This pull request is part of the [FC-0012 project](https://openedx.atlassian.net/l/cp/XGS0iCcQ) which is sparked by the [Translation Infrastructure update OEP-58](https://open-edx-proposals.readthedocs.io/en/latest/architectural-decisions/oep-0058-arch-translations-management.html#specification).
@OmarIthawi 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
openedx#353 it's no longer needed because we can pull with glob patterns now see https://github.com/openedx/openedx-atlas/blob/main/docs/decisions/0001-support-glob-pattern.rst Refs: FC-0012 OEP-58
In the
edx-platform
installed XBlocks vary from one installation to another. Therefore, hardcoding the XBlock or plugin list in a Makefile would be cumbersome. Alternatively, the XBlock information is available within thexblock.v1
Python entry points.When running
make pull_translations
in the edx-platform, the repository name of the installed XBlock isn't available. On the other hand XBlock's python module name is available.Rejected alternatives
edx-platform
Makefile and allow maintainers to override the variable name e.g.ATLAS_XBLOCK_REPOS
: Similar information is available in thexblock.v1
Python entry points.DragAndDropBlock.repo_name == "xblock-drag-and-drop-v2"
: Similar information is available in thexblock.v1
Python entry points.TODO
References
This pull request is part of the FC-0012 project which is sparked by the Translation Infrastructure update OEP-58.
Up-to-date project overview and details are available in the Approach Memo and Technical Discovery: Translations Infrastructure Implementation document.
Join the conversation on Open edX Slack #translations-project-fc-0012.
Check the links above for full information about the overall project.
Internalization is being rearchitected in Open edX Python, XBlock, Micro-frontend, and other projects. There are a number of immediately visible changes:
.json
,.po
or.mo
files will be committed into the repos.make extract_translations
in all repositoriesBreaking Changes
One of the primary goals of the project is to avoid breaking changes. If you noticed any suspicious code, please raise your concern. But before that, please know the strategy we're following to avoid breaking changes
For XBlocks:
conf/locale
. Therefore, we are creating a link fromconf/locale
totranslations
so Atlas can find the path without disrupting the current way of having translations locally within the XBlocksFor Micro-frontends:
src/i18n/index.js
atlas
integration inmake pull_translations
but only ifOPENEDX_ATLAS_PULL
is setintl-imports.js
to generate up to date import files