openedx / i18n-tools

Tools to help with internationalization and localization of Open edX projects
Apache License 2.0
26 stars 31 forks source link

feat: restore PO-Revision-Date but with fixed value #140

Closed shadinaif closed 1 year ago

shadinaif commented 1 year ago

External packages are forcing validation on PO-Revision-Date. Therefore, we are restoring both POT-Creation-Date and PO-Revision-Date but setting a fixed value for them to avoid having a lot of false (git diff) lines. Note: (2023-06-13 is Palm release date)

This is a fix for a defect reported in credentials repository: https://github.com/openedx/i18n-tools/issues/131

Reverts part of the work done here https://github.com/openedx/i18n-tools/pull/129

This contribution is part of the FC-0012 project which is sparked by the Translation Infrastructure update OEP-58.

openedx-webhooks commented 1 year ago

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.

codecov[bot] commented 1 year ago

Codecov Report

All modified lines are covered by tests :white_check_mark:

Comparison is base (ac49869) 99.77% compared to head (f1ba793) 99.78%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #140 +/- ## ========================================== + Coverage 99.77% 99.78% +0.01% ========================================== Files 10 11 +1 Lines 439 461 +22 Branches 31 33 +2 ========================================== + Hits 438 460 +22 Partials 1 1 ``` | [Files](https://app.codecov.io/gh/openedx/i18n-tools/pull/140?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openedx) | Coverage Δ | | |---|---|---| | [tests/test\_extract.py](https://app.codecov.io/gh/openedx/i18n-tools/pull/140?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openedx#diff-dGVzdHMvdGVzdF9leHRyYWN0LnB5) | `100.00% <100.00%> (ø)` | | | [tests/test\_integration.py](https://app.codecov.io/gh/openedx/i18n-tools/pull/140?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openedx#diff-dGVzdHMvdGVzdF9pbnRlZ3JhdGlvbi5weQ==) | `100.00% <100.00%> (ø)` | |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

shadinaif commented 1 year ago

Ready for review @OmarIthawi @brian-smith-tcril

OmarIthawi commented 1 year ago

Thanks @shadinaif!! Some tests are failing. Would you mind taking a look?

Also I think it makes sense to have an integration test case which runs the following two commands on a sample repo test folder:

cd tests/data/mock-django-app
# Needs some python files to generate languages for
i18n_tool extract
i18n_tool validate

I think this will ensure that this error will not happen again, what do you think?

awais786 commented 1 year ago

@OmarIthawi any thing left in this PR ? waiting for new release to unblock credentials.

OmarIthawi commented 1 year ago

Thanks for checking @awais786. We need someone to test it on the credentials repo, I couldn't get to this in a timely manner.

Justin was going to test it, but please feel free to do so.

If it's good, I can merge and cut a new release.

awais786 commented 1 year ago

@OmarIthawi https://github.com/openedx/credentials/pull/2221 plz check this. Seems trans are working,

awais786 commented 1 year ago

now credentials failing with black issue. I have fixed black path in credential so all set good to go.

black --check .

would reformat /home/runner/work/credentials/credentials/src/edx-i18n-tools/i18n/branch_cleanup.py
would reformat /home/runner/work/credentials/credentials/src/edx-i18n-tools/i18n/__init__.py
would reformat /home/runner/work/credentials/credentials/src/edx-i18n-tools/i18n/changed.py
would reformat /home/runner/work/credentials/credentials/src/edx-i18n-tools/i18n/converter.py
would reformat /home/runner/work/credentials/credentials/src/edx-i18n-tools/i18n/config.py
would reformat /home/runner/work/credentials/credentials/src/edx-i18n-tools/i18n/execute.py
would reformat /home/runner/work/credentials/credentials/src/edx-i18n-tools/i18n/dummy.py
would reformat /home/runner/work/credentials/credentials/src/edx-i18n-tools/i18n/extract.py
would reformat /home/runner/work/credentials/credentials/src/edx-i18n-tools/i18n/generate.py
would reformat /home/runner/work/credentials/credentials/src/edx-i18n-tools/i18n/main.py
would reformat /home/runner/work/credentials/credentials/src/edx-i18n-tools/i18n/segment.py
would reformat /home/runner/work/credentials/credentials/src/edx-i18n-tools/i18n/transifex.py
would reformat /home/runner/work/credentials/credentials/src/edx-i18n-tools/scripts/podup.py
would reformat /home/runner/work/credentials/credentials/src/edx-i18n-tools/setup.py
would reformat /home/runner/work/credentials/credentials/src/edx-i18n-tools/i18n/validate.py
would reformat /home/runner/work/credentials/credentials/src/edx-i18n-tools/tests/__init__.py
would reformat /home/runner/work/credentials/credentials/src/edx-i18n-tools/tests/test_config.py
would reformat /home/runner/work/credentials/credentials/src/edx-i18n-tools/tests/test_changed.py
would reformat /home/runner/work/credentials/credentials/src/edx-i18n-tools/tests/test_converter.py
would reformat /home/runner/work/credentials/credentials/src/edx-i18n-tools/tests/test_dummy.py
would reformat /home/runner/work/credentials/credentials/src/edx-i18n-tools/tests/test_extract.py
would reformat /home/runner/work/credentials/credentials/src/edx-i18n-tools/tests/test_generate.py
would reformat /home/runner/work/credentials/credentials/src/edx-i18n-tools/tests/test_integration.py
would reformat /home/runner/work/credentials/credentials/src/edx-i18n-tools/tests/test_transifex.py
would reformat /home/runner/work/credentials/credentials/src/edx-i18n-tools/tests/test_segment.py
would reformat /home/runner/work/credentials/credentials/src/edx-i18n-tools/tests/test_validate.py
justinhynes commented 1 year ago

I wasn't able to get to this yesterday. I tried it out this morning and I was able to validate translations locally without any further issues. Thanks for the work on this!

awais786 commented 1 year ago

@OmarIthawi plz release new version.

justinhynes commented 1 year ago

@awais786 That's interesting... Black shouldn't be trying to reformat the files of an external library. 🤔 Worst case maybe we just need to update pyproject.toml to ensure it's properly excluding directories? Or maybe it's a symptom of how the library was installed?

Either way, that problem is definitely on the Credentials side and I'm sure we can figure out a way to work around it.

awais786 commented 1 year ago

@awais786 That's interesting... Black shouldn't be trying to reformat the files of an external library. 🤔 Worst case maybe we just need to update pyproject.toml to ensure it's properly excluding directories? Or maybe it's a symptom of how the library was installed?

Either way, that problem is definitely on the Credentials side and I'm sure we can figure out a way to work around it.

I have already fixed https://github.com/openedx/credentials/pull/2221

openedx-webhooks commented 1 year ago

@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.

omar-nelc commented 1 year ago

@awais786 thanks for the review and the release cut. I was about to merge and cut the release, but you've beat me to it.

Thanks again for the thorough report! It's making this tool much better :)

awais786 commented 1 year ago

@OmarIthawi Thanks for the quick fix.