Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Annotation refactor#2351
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
Annotation refactor#2351
Uh oh!
There was an error while loading.Please reload this page.
Conversation
One test failed on 3.3 and I think it is unrelated to these changes. Everything else passed. |
@mdboom Can you take a look at this? |
| # property so we don't break everything that assumed this existed | ||
| textcoords = property(_get_anncoords, _set_anncoords) | ||
| anncoords = property(_get_anncoords, _set_anncoords) |
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.
We should probably use the decorator syntax for properties. We only support back to Python 2.6 now, so it's available everywhere.
If xytext is being deprecated, we should add our deprecated decorator to those so the user will get feedback.
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 wanted some feed back on if it was a good idea to change text* -> ann* and then make the sub classes provide the proper getter/setter properties. Not sure I am comfortable unilaterally changing the api with no discussion.
This looks fine (other than my style comments). It will need a test, and an entry in |
What sort of test do you want here? Would it be enough to just get the extent before and after a move or do you want an image test here? |
I think the test can just check the extent before and after the move. I think it's fine to change the API in this case, but we should go one cycle deprecating the old before removing it. The |
Got the tests working. Re-based on to master to pick up the locale fix so travis will run on 2.x Do you want any of these commits squashed? |
Travis failures are un-related to this (tight bounding box on 2.7 and a strange syntax error on 3.2). |
Does this need anything else to get merged? |
It looks like it needs a rebase -- that will give Travis one more go at it. |
All of the work done in _get_xy_legacy can be done viatransforms returned by _get_xy_transform.Removed the sole usage of this function and the function.
secondary object is to the sub-classes.Added properties to maintain back-compatibilityadded properties to replace xytext and textcoords (xyann, anncoords)properly deprecated `textcoord` and `xytext` in favor of `anncoords`and `xyann`.
re-based I squashed some of the commits. |
Deprecated in PRmatplotlib#2351Merged to master asda6c6b5
Deprecated in PRmatplotlib#2351Merged to master asda6c6b5
Deprecated in PRmatplotlib#2351Merged to master asda6c6b5
…rds"This reverts commitd6e1577, reversingchanges made tob280f7d.For 1.4.0 tacaswell refactored annotations (matplotlib#2351 ) but missed that{'axes points', 'axes pixel', 'figure points', 'figure pixel'} werespecial cased to wrap. Presumably from the function name to maintainback compatibility. This was an unintentional and undocumented APIbreak.This API break was noticed inmatplotlib#4012 and fixed inmatplotlib#4019 but that catchestoo many of the coordinate systems (should not be all things that startwith 'axes') so fixed one API, but broke others.There were two reasonable courses of action: 1. revert back to 1.4.2 behavior with nothing wrapping. 2. revert back to 1.3.1 behavior with somethings wrapping.In the discussion inmatplotlib#4292 where the consensus was to go withoption 1, hence this reversion.
This is codifying the API change that went it inmatplotlib#2351
Addresses issue#2329
The
xyannandanncoordsproperties (if people think that is a good thing to rename them to) should probably be pushed up to the_AnnotationBaselevel.