Conversation
Old as in: currently on AP.
|
OK, I think I've got everything? |
Julian
left a comment
Julian
left a comment
There was a problem hiding this comment.
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 :)
Julian
left a comment
Julian
left a comment
There was a problem hiding this comment.
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!
|
Thank you so much Julian, I realize this PR looks daunting and I appreciate your trooped through! |
|
Glad to see my opinion was not in fact required 😄 . Also, nice to see progress on this! |
3 participants
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,orderwill become False by default.