Skip to content

[Django 4.1+] Make i18n works for LANGUAGE_CODE with a country code #206

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

Merged
merged 5 commits into from
May 23, 2023

Conversation

rsebille
Copy link

Hello 👋,

When using a LANGUAGE_CODE with a country code and a supported language (ie: fr-FR) the current implementation of Select2Mixin.i18n_name will return None which means default select2 language, so English.

Not sure if testing against django.VERSION is the way to go but since the property already rely on Django code and that no one reported the current behavior, it seems a good enough way to target the versions with the fix than copying it.

Copy link
Owner

@codingjoe codingjoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @rsebille,

Thank you for your contribution. It's much appreciated. Your patch looks pretty perfect. I'd just with to have a separate test, with a little docstring to avoid future regression. We try to keep the test suite more atomic going forward, and I'd appreciate if you could contribute to that trend as well :)

Best! Joe

@codecov
Copy link

codecov bot commented Mar 23, 2023

Codecov Report

Patch coverage: 75.00% and project coverage change: -2.24 ⚠️

Comparison is base (c309742) 99.27% compared to head (c5dd7dc) 97.03%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #206      +/-   ##
==========================================
- Coverage   99.27%   97.03%   -2.24%     
==========================================
  Files           7        7              
  Lines         276      270       -6     
==========================================
- Hits          274      262      -12     
- Misses          2        8       +6     
Impacted Files Coverage Δ
django_select2/forms.py 96.93% <75.00%> (-3.07%) ⬇️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@rsebille rsebille force-pushed the fix_i18n_name_for_recent_django branch from 5be9482 to 389de3c Compare March 27, 2023 14:06
@rsebille
Copy link
Author

Hi,

While moving the new test cases into their own functions I saw TestSelect2Mixin.widget_cls and tried to use it, but it doesn't work well for TestHeavySelect2Mixin because of the required parameters (data_view or data_url) of HeavySelect2Widget() .
As it kind of bother me to have the exact same test cases executed multiple times because of the classes inheritance, which also give a false sense of security, I moved everything out and descended into the pytest rabbit hole as it seems easier than starting to touch every tests in the file :).

I looked to the last failed CI jobs but didn't succeed to reproduce in local nor found an obvious link between those and the proposed changes so hopefully it was some temporary hiccups on github side ;).

@rsebille rsebille requested a review from codingjoe May 2, 2023 15:47
@codingjoe codingjoe merged commit ed1af30 into codingjoe:main May 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants