Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Deprecate case-insensitive properties.#16987
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
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 ultimatelyimprove 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 usingarrow.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 applymixed_case_parameters = ['UVC', 'patchA', 'patchB']if k != k.lower() and k not in mixed_case_parameters:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Butset(patcha=foo) didn't actually work before (and still fails with this patch). The only difference is thatset(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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Well ok, While I think there must be a better solution, likely nobody will notice if this has been broken currently.
7515f4f tobd6fe5cCompare| Case-insensitive properties | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
| Support for upper or mixed-case property names in `.Artist.set` and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
IfpatchA is now accepted, then this statement is incorrect now, isn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Right, but this writeup says the opposite (even it didn't work before.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I see, the wording was bad, pushed a different one, how does that look?
bd6fe5c tof3a2cffCompareAs a real case, incorrectly passing "axA" instead of "axesA" toConnectionPatch currently triggers a slightly confusing warningregarding the non-existence of the "axa" property.
f3a2cff toaa85070Comparerebased |
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