-
-
Notifications
You must be signed in to change notification settings - Fork 63
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
base: main
Are you sure you want to change the base?
Conversation
Locales starting with en_* will default to no transaltion.
for more information, see https://pre-commit.ci
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #230 +/- ##
==========================================
- Coverage 99.47% 97.95% -1.52%
==========================================
Files 11 11
Lines 760 783 +23
==========================================
+ Hits 756 767 +11
- Misses 4 16 +12
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? |
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.
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? |
The linter does not understand windows powershell syntax. So we move it to the scripts folder.
Fixes #210
Replaces existing PR #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: