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

Make order=False by default? #104

Closed
alanhdu opened this issue Dec 1, 2017 · 6 comments
Closed

Make order=False by default? #104

alanhdu opened this issue Dec 1, 2017 · 6 comments

Comments

@alanhdu
Copy link

alanhdu commented Dec 1, 2017

This is kind of a small nit, but I'm wondering whether the order parameter should be False by default instead of True? I suspect that most dataclasses don't have a natural ordering associated with them and it'd be better to throw a TypeError rather than returning at all.

Thanks for creating this library though! I'm can't wait to see this in the Python standard library!

@ericvsmith
Copy link
Owner

If the fields values are unorderable, you do get a TypeError:

@dataclass
class C:
    i: int
    c: complex

print(C(0, 1j) < C(0, -2j))

Traceback (most recent call last):
  File "i.py", line 44, in <module>
    print(C(0, 1j) < C(0, -2j))
  File "<string>", line 3, in __lt__
TypeError: '<' not supported between instances of 'complex' and 'complex'

Which is slightly worse that what you get with order=False:

@dataclass(order=False)
class C:
    i: int
    c: complex

print(C(0, 1j) < C(0, -2j))

Traceback (most recent call last):
  File "i.py", line 44, in <module>
    print(C(0, 1j) < C(0, -2j))
TypeError: '<' not supported between instances of 'C' and 'C'

I don't have a feel for how often instances will be ordered. But it's true that I don't often try to order attrs instances. I guess attrs doesn't have this issue because they group our eq and order together in their cmp, and you definitely want __eq__ and __ne__ by default.

My gut feeling is that we should leave order=True as the default if comparing unorderable fields raises TypeError.

If no one chimes in here with an opinion, I'll raise this on python-dev (or feel free to do that yourself).

@alanhdu
Copy link
Author

alanhdu commented Dec 1, 2017

Hm... I'm more worried about technically orederable field values that shouldn't be ordered (e.g. an integer ID field) than an unorderable field value like a complex number. Complex numbers are actually a good example -- they're composed of orderable fields (floats) but don't really have an order themselves.

@gvanrossum
Copy link

gvanrossum commented Dec 1, 2017 via email

@ericvsmith
Copy link
Owner

With this change, the default would be that you can't order classes, but when you compare them they compare by value, not identity (as is the default for regular classes). I think that's a reasonable default for something focused on the fields within classes.

@gvanrossum
Copy link

gvanrossum commented Dec 1, 2017 via email

@ericvsmith
Copy link
Owner

Thanks for raising this issue, @alanhdu!

ericvsmith added a commit to python/peps that referenced this issue Dec 2, 2017
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

No branches or pull requests

3 participants