Skip to content
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

Country implements ordering #361

Merged
merged 8 commits into from Feb 2, 2019
Merged

Country implements ordering #361

merged 8 commits into from Feb 2, 2019

Conversation

TrilceAC
Copy link
Contributor

Country implements a total ordering based on Country.code. This ordering is also comparable to any string. This solves #360

Carlos García Montoro added 2 commits January 29, 2019 17:05
Country needs to be orderable if it is part of the primary key and cascade deletion is in use.
@kvesteri
Copy link
Owner

This needs at least couple of unit tests. Also why not use total_ordering decorator from functools module?

@TrilceAC
Copy link
Contributor Author

I'll try to prepare some tests.

Regarding the use of tolta_ordering, if you want, I can use it. I did not just because of the note at total_ordering, and because it was easy to implemente the four methods

Note: While this decorator makes it easy to create well behaved totally ordered types, it does come at the cost of slower execution and more complex stack traces for the derived comparison methods. If performance benchmarking indicates this is a bottleneck for a given application, implementing all six rich comparison methods instead is likely to provide an easy speed boost.

Probably performance will never be an issue, but every little bit helps.

@kvesteri
Copy link
Owner

I'm all for code clarity and simplicity over micro-optimizations, hence please use total_ordering even if there is a small performance penalty (as there is with any decorators).

For tests you can just have single test method parametrized

@pytest.mark.parametrize(
   'assertion',
   [
       Country('FI') > Country('ES'),
       ...
   ]
)
def test_ordering(assertion):
    assert assertion

@TrilceAC
Copy link
Contributor Author

I have implemented the changes that you propose. Please check.

@kvesteri kvesteri merged commit e241b54 into kvesteri:master Feb 2, 2019
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.

None yet

2 participants