-
-
Notifications
You must be signed in to change notification settings - Fork 64
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
Handle en_GB and en_US locale #230
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #230 +/- ##
==========================================
+ Coverage 99.47% 99.49% +0.01%
==========================================
Files 11 11
Lines 760 785 +25
==========================================
+ Hits 756 781 +25
Misses 4 4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Appreciated! What's the reason for removing |
The current type hint for def activate(
locale: str, path: str | os.PathLike[str] | None = None
) -> gettext_module.NullTranslations: So If this is desired though, it's absolutely OK to amend the type hint. I will then also need to slightly amend the function implementation to deal with I don't really see the reason for I'll let you decide what you think is best. |
Yes, these are good points. I do see a couple of uses of https://github.com/search?q=%2Factivate%5C%28None%2F&ref=opensearch&type=code But we don't know how much non-GitHub code might do What do you think? |
I fully agree. It seems like a good idea to allow I made the change and I moved to test inside the I then noticed that the original test had a small issue. It did not use the decorator I decided to reduce this gotcha effect by making |
My unit tests were failing on Windows because The unit tests now pass on Windows but I'm getting
For code coverage, I'm looking on Sentry, but the only lines that are not tested seem to be the docstring I added to the pytest fixture. What am I missing? |
39a9308
to
97afe65
Compare
Let's not bother with the . (And I've noticed it was accidentally left out when adding the automation, I've reported this in #231.) |
Hi, I'm happy to remove the Unless you want me to also change the test so we never try to translate anything in the unittests? I noticed in other tests this pattern try:
humanize.i18n.activate("ru_RU")
assert humanize.naturaltime(three_seconds) == "3 секунды назад"
assert humanize.ordinal(5) == "5ый"
assert humanize.precisedelta(one_min_three_seconds) == "1 минута и 7 секунд"
except FileNotFoundError:
pytest.skip("Generate .mo with scripts/generate-translation-binaries.sh") Is it how we want to handle it? No unittests should run translations on the CI? |
Yes, let's use the We can see skips for the Windows CI: But they're run on the macOS and Ubuntu CI:
Because we're installing humanize/.github/workflows/test.yml Lines 28 to 43 in de8b8a1
This means people can also test locally without generating the binaries. |
Locales starting with en_* will default to no transaltion.
for more information, see https://pre-commit.ci
This is similar to calling i18n.deactivate.
The global variable NOW came with a gotcha. When using it, it was also needed to decorate the test with freeze_time. NOW is now a fixture which keeps the time frozen for the duration of the test using it.
The linter does not understand windows powershell syntax. So we move it to the scripts folder.
Update the unittest to be skipped in case the translation binaries were missing.
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 for your patience, I think we're almost there!
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.
Thank you very much!
Fixes #210
Replaces existing PR and closes #222
I tried to push that PR over the finishing line. The commits could all be squashed in a single commit. I wanted to keep track of the original author contribution.
Changes proposed in this pull request: