-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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: FC-0012 - add (--no-segment) option to extract_translations #33368
feat: FC-0012 - add (--no-segment) option to extract_translations #33368
Conversation
Thanks for the pull request, @shadinaif! 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. |
0f2dd62
to
30e148a
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 @shadinaif. I realize this is still work-in progress but I'm adding notes if it's useful.
fb42958
to
f20c748
Compare
Thanks @shadinaif. Some tests are failing. Sometimes we only need to rebase over |
f20c748
to
e55be0e
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 @shadinaif!
This looks great!
Would you mind checking my last note? Same code but one less line.
Makefile
Outdated
cd conf/locale/en/LC_MESSAGES && msgcat djangojs.po underscore.po -o djangojs.po | ||
cd conf/locale/en/LC_MESSAGES && msgcat django.po wiki.po edx_proctoring_proctortrack.po mako.po -o django_temp.po | ||
cd conf/locale/en/LC_MESSAGES && mv django_temp.po django.po | ||
cd conf/locale/en/LC_MESSAGES && rm wiki.po edx_proctoring_proctortrack.po mako.po underscore.po |
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.
cd conf/locale/en/LC_MESSAGES && msgcat djangojs.po underscore.po -o djangojs.po | |
cd conf/locale/en/LC_MESSAGES && msgcat django.po wiki.po edx_proctoring_proctortrack.po mako.po -o django_temp.po | |
cd conf/locale/en/LC_MESSAGES && mv django_temp.po django.po | |
cd conf/locale/en/LC_MESSAGES && rm wiki.po edx_proctoring_proctortrack.po mako.po underscore.po | |
cd conf/locale/en/LC_MESSAGES && msgcat djangojs.po underscore.po -o djangojs.po | |
cd conf/locale/en/LC_MESSAGES && msgcat django.po wiki.po edx_proctoring_proctortrack.po mako.po -o django.po | |
cd conf/locale/en/LC_MESSAGES && rm wiki.po edx_proctoring_proctortrack.po mako.po underscore.po |
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.
done
Makefile
Outdated
# (IS_OPENEDX_TRANSLATIONS_WORKFLOW) is set to "yes" when openedx-translations calls this target. | ||
# Otherwise, we preserve the old behavior of having segmented po files. |
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_OPENEDX_TRANSLATIONS_WORKFLOW) is set to "yes" when openedx-translations calls this target. | |
# Otherwise, we preserve the old behavior of having segmented po files. | |
# (IS_OPENEDX_TRANSLATIONS_WORKFLOW) is set to "yes" when openedx-translations repository extracts translations. | |
# Related doc: https://docs.openedx.org/en/latest/developers/how-tos/enable-translations-new-repo.html | |
# Otherwise, we preserve the old behavior of having segmented po files. |
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.
done
@shadinaif I've added two minor notes. Please test it on the |
c028dfa
to
d6fc155
Compare
Tested, and ready to be merged @OmarIthawi @brian-smith-tcril |
@jmbowman this is another FC-0012 pull request that needs your review. |
@shadinaif please add the |
d6fc155
to
dac8523
Compare
@@ -37,8 +37,19 @@ technical-docs: ## build the technical docs | |||
guides: swagger ## build the developer guide docs | |||
cd docs/guides; make clean html | |||
|
|||
# (IS_OPENEDX_TRANSLATIONS_WORKFLOW) is set to "yes" when openedx-translations repository extracts translations. | |||
# Related doc: https://docs.openedx.org/en/latest/developers/how-tos/enable-translations-new-repo.html |
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 clear reading through this doc how it is related to this. I think it makes more sense to link to https://github.com/openedx/edx-platform/blob/59ddee7ad3363bf9f4bfae355fa3f24a1bbdfb37/docs/decisions/0018-standarize-django-po-files.rst once it is merged instead.
@dianakhuang would you mind taking a look on this pull request? @shadinaif please let me know if there's anything we can do to push the PR forward. |
@shadinaif would you mind checking the notes in the review? |
dac8523
to
096798e
Compare
The flag is added if and only if (IS_OPENEDX_TRANSLATIONS_WORKFLOW) is set to "yes" Refs: FC-0012 OEP-58
096798e
to
3b26280
Compare
Tested here @OmarIthawi , please 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.
Looks great! Thanks @shadinaif!
@dianakhuang you're listed as "Review required from" for this repo on the spreadsheet. Any chance you could review/merge this one? |
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 @brian-smith-tcril ! It was on my radar, but I figured I'd let everyone else get to it first. Looks good to me.
@shadinaif 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
2U Release Notice: This PR has been deployed to the edX production environment. |
Description
Add
--no-segment
option toextract_translations
. The flag is added if and only ifIS_OPENEDX_TRANSLATIONS_WORKFLOW
is set toyes
. This a follow-up work to Decision to standardize django/djangojs po filesIS_OPENEDX_TRANSLATIONS_WORKFLOW
flag is introduced and used by openedx-translationsReferences
This contribution 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 re-architected 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 notice any suspicious code, please raise your concern. But before that, please know the strategy we're following to avoid breaking changes