-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
API : tighten validation on pivot in Quiver #3955
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
Conversation
@@ -405,6 +405,8 @@ class Quiver(mcollections.PolyCollection): | |||
in the draw() method. | |||
""" | |||
|
|||
_PIVOT_VALS = {'tail', 'tip', 'mid', 'middle'} |
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.
Should be a tuple.
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 intentionally made it a set as the order has no meaning.
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 know, but the set syntax is why it is failing on Python 2.6. A set confers no advantages over a tuple or list in this use case. The {}
set literal syntax was backported from 3.1 to 2.7 only.
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.
Ah, I am showing my lack of age and have never used a version of python that did not have {}
for set syntax. Also was being lazy and assumed that the the 2.6 failure was that speed-up test failing.
I can toss the comment-related PEP8 if it annoys people/is adding too much noise/code churn. |
@tacaswell, in this case I have no objection to leaving in the PEP8 fixes. |
Looks good. Do you want to squash it, or should I go ahead and merge? |
I will squash it, this is a rather embarrassing set of commits. On Mon, Dec 29, 2014, 17:52 Eric Firing notifications@github.com wrote:
|
only accept {'mid', 'middle', 'tip', 'tail'} instead of being super permissive. Closes matplotlib#3951
Fixes 'violations' we don't test for, all white space in comments.
e03eca4
to
e6419cf
Compare
@efiring Squashed (and incidentally rebased on master) |
API : tighten validation on pivot in Quiver
In PR matplotlib#3955 / af17051 the quiver pivot location was normalized to be 'tip', 'tail', or 'middle' internally. The key 'mid' is now normalized to 'middle'. The quiver key code circumvents the normalization and was setting `Quiver.pivot' to 'mid', which fell back to the default behavior of pivoting on the tail when the quiver key is drawn. closes matplotlib#5613
only accept {'mid', 'middle', 'tip', 'tail'} instead of
being super permissive.
Closes #3951