Deprecate case-insensitive properties. #16987
Conversation
I'm not going to argue against case sensitivity, but the warning could also be made more clear by keeping a copy of |
|
Do we have any non-lowercase properties? If so that would pose a consistency problem. Otherwise, I agree with the proposednchange. |
Grepping shows there's FancyArrowPatch's patchA/patchB and Quiver's UVC: I guess if anything this PR will ultimately improve the situation for them as it will allow |
| @@ -980,7 +980,12 @@ def update(self, props): | ||
| ret = [] | ||
| with cbook._setattr_cm(self, eventson=False): | ||
| for k, v in props.items(): | ||
| k = k.lower() | ||
| if k != k.lower(): |
There was a problem hiding this comment.
it will allow
arrow.set(patchA=foo)which failed so far?
Yes, that will work, but give a confusing warning "Case-insensitive properties were deprecated ...", which you can silence with using arrow.set(patcha=foo), which in turn will break after the deprecation.
can we make this
# excluding mixed case parameters from the deprecation as it does not apply
mixed_case_parameters = ['UVC', 'patchA', 'patchB']
if k != k.lower() and k not in mixed_case_parameters:
There was a problem hiding this comment.
But set(patcha=foo) didn't actually work before (and still fails with this patch). The only difference is that set(patchA=...) used to fail and now will fail with a slightly more confusing error message, but will work after the deprecation.
There was a problem hiding this comment.
Well ok, While I think there must be a better solution, likely nobody will notice if this has been broken currently.
7515f4f to
bd6fe5c
Compare
|
|
||
| Case-insensitive properties | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
| Support for upper or mixed-case property names in `.Artist.set` and |
There was a problem hiding this comment.
If patchA is now accepted, then this statement is incorrect now, isn't it?
There was a problem hiding this comment.
It's not accepted now (and wasn't before) (as it was and is still wrongly normalized to patcha), but will be after the deprecation.
There was a problem hiding this comment.
Right, but this writeup says the opposite (even it didn't work before.)
There was a problem hiding this comment.
I see, the wording was bad, pushed a different one, how does that look?
bd6fe5c to
f3a2cff
Compare
As a real case, incorrectly passing "axA" instead of "axesA" to ConnectionPatch currently triggers a slightly confusing warning regarding the non-existence of the "axa" property.
f3a2cff to
aa85070
Compare
|
rebased |
As a real case, incorrectly passing "axA" instead of "axesA" to
ConnectionPatch currently triggers a slightly confusing warning
regarding the non-existence of the "axa" property.
PR Summary
PR Checklist