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

Split cmp into eq and order #574

Merged
merged 13 commits into from Sep 22, 2019
Merged

Split cmp into eq and order #574

merged 13 commits into from Sep 22, 2019

Conversation

hynek
Copy link
Member

@hynek hynek commented Sep 16, 2019

Fixes #170

Whoa, this is a doozy. People have been asking for this for a long time and I really wanted it too. So here it is, with a lot more changes that you'd expect.

Since this one is rather radical, I think we'll give it at least 18 months of deprecation period. I also think that I'll add a sys.version_info-style version number so people can easily write warnings-free code that won't break.

This is a very important stepping stone towards detect_own (which implies @attr.auto/import attrs) and pluggable method factories. In auto/import attrs, order will become False by default.

tests/test_dark_magic.py Outdated Show resolved Hide resolved
@hynek
Copy link
Member Author

hynek commented Sep 18, 2019

OK, I think I've got everything?

@hynek hynek added this to the 19.2 milestone Sep 18, 2019
Copy link
Member

@Julian Julian left a comment

Choose a reason for hiding this comment

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

Left a few more comments -- one "key" approach question.

And another general one -- which is a bunch of the code in the tests basically ports tests over from testing cmp to testing eq+order, rather than say, maintaining those tests, and running them over both settings but parametrized for each.

Seems possibly slightly dangerous to do that because this code will stick around for awhile, and presumably we want cmp to keep working over that period, even if there are modifications just to its code path -- but I know it's a giant PITA to inject that kind of test parametrization post-hoc, so maybe it's acceptable risk...

But overall... even though this is now getting a bit large for me to be personally confident about, I think this looks pretty good to me at least :)

docs/hashing.rst Outdated Show resolved Hide resolved
docs/hashing.rst Outdated Show resolved Hide resolved
src/attr/_make.py Show resolved Hide resolved
src/attr/_make.py Show resolved Hide resolved
src/attr/_make.py Outdated Show resolved Hide resolved
tests/test_dark_magic.py Show resolved Hide resolved
Copy link
Member

@Julian Julian left a comment

Choose a reason for hiding this comment

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

OK! There's one last straggling reference that possibly is worth updating in docs/extending.rst that shows usage of cmp when maybe it should show eq now (typed function in the Metadata section), but other than that lgtm!

@hynek
Copy link
Member Author

hynek commented Sep 22, 2019

Thank you so much Julian, I realize this PR looks daunting and I appreciate your trooped through!

@hynek hynek merged commit 08fcfe9 into master Sep 22, 2019
@hynek hynek deleted the split-cmp branch September 22, 2019 13:07
@glyph
Copy link
Contributor

glyph commented Sep 24, 2019

Glad to see my opinion was not in fact required 😄 .

Also, nice to see progress on this!

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.

Suggestion: differentiate in cmp between (== , !=) and (>,>=, <, <=).
3 participants