feat: add new options merge-po-files and no-segment #134
Conversation
openedx-webhooks
added
the
open-source-contribution
|
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. |
shadinaif
force-pushed
the
shadinaif/new-options-for-extract
branch
from
30c31f8
to
b90f379
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #134 +/- ##
==========================================
+ Coverage 99.75% 99.77% +0.02%
==========================================
Files 10 10
Lines 401 439 +38
Branches 27 31 +4
==========================================
+ Hits 400 438 +38
Partials 1 1
☔ View full report in Codecov by Sentry. |
|
This one is ready @OmarIthawi @brian-smith-tcril , also tested on another package to ensure it's validity |
There was a problem hiding this comment.
@shadinaif I'm having a bit of a hard time in reviewing this PR.
Would it be too much to ask if we split into two smaller PRs?
I understand that there's a lot of code moved between files, but I would need at least an hour t to review it. Others in the future looking at the code will also need to spent similar time.
Please let me know what do you recommend? (A) Review as-is (B) Split.
shadinaif
force-pushed
the
shadinaif/new-options-for-extract
branch
from
b90f379
to
d58e142
Compare
There was a problem hiding this comment.
Thanks @shadinaif. Would you mind addressing the notes I've written.
The directions is good and I think it makes sense to move forward with it to simplify the XBlock and small repo configurations.
However, I think it's worth paying attention to the developer experience of the tool because very few engineers actually understand the merge/combine logic here and the options we're introducing aren't easy to understand.
Therefore I'm suggesting an interface change.
shadinaif
force-pushed
the
shadinaif/new-options-for-extract
branch
from
d58e142
to
f5bcde1
Compare
OmarIthawi
changed the title
OmarIthawi
changed the title
shadinaif
force-pushed
the
shadinaif/new-options-for-extract
branch
from
f5bcde1
to
eaf0097
Compare
Test 1: Drag and drop xblock ✅CLI test logIt works great, but leaves the following side-effects:
Admittedly, this behavior isn't going to break anything so it's perhaps clunky but okay to have since XBlocks needs to explicitly use the new flags and those XBlocks should avoid adding the Test 2: LTI Consumer XBlock ✅CLI commands and results |
There was a problem hiding this comment.
Works great. Thanks @shadinaif.
I'll ask @sarina or @brian-smith-tcril for a quick review to spot check wordings and design just in case we missed something.
| @@ -6,7 +6,7 @@ | |||
|
|
|||
| from . import config | |||
|
|
|||
| __version__ = '1.1.1' | |||
| __version__ = '1.2.0' | |||
There was a problem hiding this comment.
I'm just curious; is this being manually updated or do we have automatic semantic versioning of some sort for this repo?
There was a problem hiding this comment.
It's suprisingly hard to answer this 😅. Here's what I've checked:
- No semantic-release integration was found: https://github.com/search?q=repo%3Aopenedx%2Fi18n-tools+semantic&type=code
- No workflow appears to support it: https://github.com/openedx/i18n-tools/tree/master/.github/workflows
- It seems to be manually updated: https://github.com/openedx/i18n-tools/commits/master/i18n/__init__.py
I've filed an issue for that:
shadinaif
changed the title
shadinaif
force-pushed
the
shadinaif/new-options-for-extract
branch
3 times, most recently
from
77275d0
to
fd75992
Compare
shadinaif
force-pushed
the
shadinaif/new-options-for-extract
branch
from
fd75992
to
88e81a9
Compare
|
Thank you @OmarIthawi , thank you @brian-smith-tcril . It is ready now |
There was a problem hiding this comment.
Thanks again @shadinaif for making the pull request.
I tested this pull request on the edx platform and here's what I've tried:
make dev.up.lms
make dev.shell.lms
pip install -e git+https://github.com/Zeit-Labs/i18n-tools.git@shadinaif/new-options-for-extract#egg=edx-i18n-tools==1.1.0
I've tried the following commands:
Step 1: Save a copy for master's po files
cp -r en en-master # save a copy
Step 2: Generate with --no-partial with the original config.yaml file
rm -rf conf/locale/en/; mkdir -p conf/locale/en/LC_MESSAGES; i18n_tool extract --no-partial; find conf/locale/en/
cp -r en en-no-partial
Step 3: Remove the segment section from the config.yaml file and run with --no-partial
rm -rf conf/locale/en/; mkdir -p conf/locale/en/LC_MESSAGES; i18n_tool extract --no-partial; find conf/locale/en/
cp -r en en-no-partial-no-segment
Step 4: Remove the segment section from the config.yaml and run without --no-partial
rm -rf conf/locale/en/; mkdir -p conf/locale/en/LC_MESSAGES; i18n_tool extract; find conf/locale/en/
cp -r en en-no-segment-with-partial
Looking at the files, I've found that the --no-partial still respects the segment section which isn't intended. So I'd like to suggest the following change:
- Rename
--no-partial-->--no-segment - Keep the current functionality in which we don't generate
partialfiles. - Additionally, the
--no-segmentoptions should discard thesegmentsection from theconfig.yaml.
I recommend checking the attached zip file or trying the results yourself:
shadinaif
force-pushed
the
shadinaif/new-options-for-extract
branch
5 times, most recently
from
3a85750
to
b739443
Compare
|
Please review again @OmarIthawi . I've tested again on both |
OmarIthawi
changed the title
There was a problem hiding this comment.
I think this is super now.
I've done extensive testing to on the named-release/olive.master branch and it produced no difference in terms of msgid which is remarkable!
Full testing script and results is available here: https://gist.github.com/OmarIthawi/3de2704804401a58c01aca1ef5ab3fb5
i18n/extract.py
Outdated
| '--no-segment', | ||
| action='store_true', | ||
| help=( | ||
| 'Renames (django-partial.po) and (djangojs-partial.po) to (django.po) and (djangojs.po) respectively. ' |
There was a problem hiding this comment.
| 'Renames (django-partial.po) and (djangojs-partial.po) to (django.po) and (djangojs.po) respectively. ' | |
| 'Ignore the `segment` section in the `config.yaml` configurations file. ' | |
| 'Renames (django-partial.po) and (djangojs-partial.po) to (django.po) and (djangojs.po) respectively. ' |
There was a problem hiding this comment.
@OmarIthawi refactored rephrased:
help=(
'Ignore the `segment` section in the `config.yaml` configurations file. Also rename '
'(django-partial.po) and (djangojs-partial.po) to (django.po) and (djangojs.po) respectively, '
'which means that the original files will be overwritten after the extraction is completed. This is '
'helpful when running the `extract` command in small repositories with no segment/merge workflow.'
)
no-segment: no partial files are generated, because they replaced the original django.po and djangojs.po. Also no segmet workflow is processed merge-po-files: no djangojs*.po is generated, because it has been merged into django*.po Refs: FC-0012 OEP-58
Bump version to include --merge-js and --no-partial options
shadinaif
force-pushed
the
shadinaif/new-options-for-extract
branch
from
b739443
to
afabc56
Compare
|
@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. |
feat: add two new options,
merge-po-filesandno-segmentAfter we passed a lot of steps in FC-0012 project; we see a need to add the following options to
extractcommand because it'll serve us in a shared logic between many other packages (especially XBlocks). So, instead of repeating the same logic in Makefiles; we just add the options. DRY, and more readable--no-segment: we have a logic inextractcommand that generates (partial files). This is needed because of the complicity ofedx-platform. But it creates a burden on small XBlocks that are always interested in the finaldjango.po. Before this option; we have to always rename the generateddjango-partial.potodjango.po. The option will also skip the segment process is any--merge-po-files: small packages (like small XBlocks) don't need to have two separate translation files for HTML and JS. We usually merge the generated filedjangojs.pointodjango.pomanually. This option will take care of that without repeating the steps in all small packagesLocal Manual Testing
I used xblock-drag-and-drop-v2 locally to test
extractcommand:1.1.0version. Done intest.infilemake upgradedjango.pofile andtext.posymbolic link fromxblock-drag-and-drop-v2/drag_and_drop_v2/conf/locale/en/LC_MESSAGESextract_translationscommand inMakefilemake extract_translations.text.pois generated successfullypy38-django32andtranslations-django32tests of xblock-drag-and-drop-v2 package. All passed