Undeprecate cmp #773
Undeprecate cmp #773
Conversation
|
Not sure which changelog file I should add. I thought it was 773, but there is already a 787. |
src/attr/_make.py
Outdated
| """ | ||
| warnings.warn(_CMP_DEPRECATION, DeprecationWarning, stacklevel=2) |
There was a problem hiding this comment.
I've been thinking and I think we should keep this deprecation.
The behavior of cmp is rather muddy, because it's a derivation of eq and order.
Now cmp becomes the officially blessed way of setting eq and order but this attribute doesn't make sense to me.
Especially because it returns a bool, while eq and order can be callables.
Does that make sense to you?
There was a problem hiding this comment.
I can see where you're coming from. We have three arguments: eq, order and cmp, so one of them is redundant for sure. At the same time, eq and order aren't entirely clean, because order requires eq.
My preference would be to undeprecate cmp to avoid attr.ib(eq='str.lower', order='eq.lower'). I think attr.ib(cmp='eq.lower') feels and looks a bit nicer.
For the record, in my applications I will use attr.ib(eq=key_func(... ), order=False). Very often order won't make sense with complex objects. In fact, I'm a lot more annoyed by order being True by default than I am by cmp vs eq.
Having said that, I completely understand that cmp deprecation has been on the cards for a long time, and that undeprecating it may feel like a regression. Happy to follow your lead.
There was a problem hiding this comment.
I think/hope we've misunderstood each other!
What I mean is that we should undeprecate cmp for attr.s/attr.attrs (and add it to attr.define for that matter, but that's a different PR) since it's syntactic sugar for setting eq/order and very useful.
But I want to remove it eventually from the Attribute class. That's why I've commented specifically on the change in the cmp property.
There was a problem hiding this comment.
Oh I see, you want to keep the deprecation specifically in the cmp property of Attribute - not else where? Sorry, my bad. Would we keep it in Attribute's init?
Especially because it returns a bool, while eq and order can be callables.
eq and order are also booleans. The callables are stored under eq_key and order_key.
There was a problem hiding this comment.
Do we need a new deprecation text?
_CMP_DEPRECATION = (
"The usage of `cmp` is deprecated and will be removed on or after "
"2021-06-01. Please use `eq` and `order` instead."
)There was a problem hiding this comment.
Would we keep it in Attribute's init?
You mean the deprecation? Yes. I would like to remove cmp from Attribute for good eventually. I want it only to be a syntactic sugar, nothing more.
There was a problem hiding this comment.
All done (I think). Apologies for the confusion.
I'm confused, what is about 787? :) Your correct issue number is 773 so make it Don't spend too much time on it; I will edit it anyways because I need to add some apologies. |
Done. |
|
Out of curiosity, what is keeping us from removing How about the property? Is it even possible to quantity the amount of damage that would be caused if we removed it? Just curious to understand how you manage breaking changes. |
Yes, but people have historically done so nevertheless. :(
It's not! Which is why SemVer is bullshit (blog post incoming) but I'm trying to be extra careful and diligent. They're both gonna move on to a better place in June.
Poorly, but I'm trying my best. :) |
|
Thanks! |
|
I'll submit another PR with the helper function soon. |
Pull Request Check List
This is just a friendly reminder about the most common mistakes. Please make sure that you tick all boxes. But please read our contribution guide at least once, it will save you unnecessary review cycles!
If an item doesn't apply to your pull request, check it anyway to make it apparent that there's nothing left to do. If your pull request is a documentation fix or a trivial typo, feel free to delete the whole thing.
versionadded,versionchanged, ordeprecateddirectives. Find the appropriate next version in our__init__.pyfile..rstfiles is written using semantic newlines.changelog.d.If you have any questions to any of the points above, just submit and ask! This checklist is here to help you, not to deter you from contributing!