-
Notifications
You must be signed in to change notification settings - Fork 95
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
Sort choices to make it easier to select #123
Conversation
1b5a399
to
70dea6e
Compare
Strengthen edge cases sorting in standard and proper GMT sorting (alpha sort does not result in a natural order with -xx:xx and +xx:xx).
|
Hi, Merge this pls. thanks. |
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 the contribution. Two minor comments that need to be addressed before this can be merged - the CI is failing because those checks were removed (unused variables).
There is a separate issue causing CI to fail - a codecoverage token issue. I'll address that via a separate PR
Let me refresh my memory on the code, I'll try to get this done quickly to keep the momentum. |
0cfa00d
to
181276b
Compare
@mfogel I found a good way to re-introduce the assert in both tests. I use sorted for speed and Counter (slower but required for non sortable elements) to validate the test. |
Awesome, thank you for all that @hrbonz I fixed the codecov token issue on |
Hi @hrbonz, can you rebase? Thanks for your time. |
Sorting the choices in forms helps with a more natural selection process
181276b
to
9bcf0ef
Compare
@mfogel @paulocoutinhox rebased and the same test is still failing. |
Here's the failing test: ___________________________________ test_specifying_defaults_not_frozen[zoneinfo] ____________________________________
use_pytz = False
to_tzobj = <bound method ZoneInfoBackend.to_tzobj of <timezone_field.backends.zoneinfo.ZoneInfoBackend object at 0x7f8343ef3a60>>
base_tzstrs = {'Africa/Abidjan', 'Africa/Accra', 'Africa/Addis_Ababa', 'Africa/Algiers', 'Africa/Asmara', 'Africa/Asmera', ...}
def test_specifying_defaults_not_frozen(use_pytz, to_tzobj, base_tzstrs):
"""
If someone's matched the default values with their kwarg args, we
shouldn't bothering freezing those
(excluding the use_pytz, which changes with your django version).
"""
field = TimeZoneField(max_length=63)
_name, _path, _args, kwargs = field.deconstruct()
assert "max_length" not in kwargs
choices = [(to_tzobj(tz), tz.replace("_", " ")) for tz in base_tzstrs]
field = TimeZoneField(choices=choices, use_pytz=use_pytz)
_name, _path, _args, kwargs = field.deconstruct()
> assert "choices" not in kwargs
E AssertionError: assert 'choices' not in {'choices': [('Pacific/Funafuti', 'Pacific/Funafuti'), ('America/Panama', 'America/Panama'), ('America/Port_of_Spain',...ersey'), ('Indian/Kerguelen', 'Indian/Kerguelen'), ('America/Goose_Bay', 'America/Goose Bay'), ...], 'use_pytz': False}
tests/test_deconstruct.py:78: AssertionError From verbose run:
I did another pass on my code and can't find a reason for my changes to break this test. For information, here's my environment:
|
Thanks for rebasing. I should have time to dig into this this weekend. |
Looks like there's still an issue with codecov |
Ok, I've fixed that failing test... it was implicitly depending on the sort order of the Thanks a bunch for the contribution @hrbonz ! Merging in now |
@mfogel do we have a minor release for that fix or can we have one ? :) |
Yes - please can we have a release for this. |
PLS!!! |
Yup sorry for the delay. I'm planning to get a new release out this weekend. |
Yay, can't wait! |
Ok I just released v7.0 to pypi. Please open an issue if you run across any issue upgrading to it, thanks! |
The sorting is applied to both standard and GMT choices.
Close #116