Skip to content
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

API : tighten validation on pivot in Quiver #3955

Merged
merged 2 commits into from Dec 30, 2014

Conversation

tacaswell
Copy link
Member

only accept {'mid', 'middle', 'tip', 'tail'} instead of
being super permissive.

Closes #3951

@tacaswell tacaswell added this to the v1.5.x milestone Dec 28, 2014
@@ -405,6 +405,8 @@ class Quiver(mcollections.PolyCollection):
in the draw() method.
"""

_PIVOT_VALS = {'tail', 'tip', 'mid', 'middle'}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be a tuple.

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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.

@tacaswell
Copy link
Member Author

I can toss the comment-related PEP8 if it annoys people/is adding too much noise/code churn.

@efiring
Copy link
Member

efiring commented Dec 29, 2014

@tacaswell, in this case I have no objection to leaving in the PEP8 fixes.

@efiring
Copy link
Member

efiring commented Dec 29, 2014

Looks good. Do you want to squash it, or should I go ahead and merge?

@tacaswell
Copy link
Member Author

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:

Looks good. Do you want to squash it, or should I go ahead and merge?


Reply to this email directly or view it on GitHub
#3955 (comment)
.

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.
@tacaswell
Copy link
Member Author

@efiring Squashed (and incidentally rebased on master)

efiring added a commit that referenced this pull request Dec 30, 2014
API : tighten validation on pivot in Quiver
@efiring efiring merged commit d92d2ad into matplotlib:master Dec 30, 2014
@tacaswell tacaswell deleted the quiver_pivot_validate branch December 30, 2014 01:03
tacaswell added a commit to tacaswell/matplotlib that referenced this pull request Dec 5, 2015
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

validation of pivot in quiver
2 participants