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

Annotation refactor #2351

Merged
merged 4 commits into from Sep 10, 2013
Merged

Annotation refactor #2351

merged 4 commits into from Sep 10, 2013

Conversation

tacaswell
Copy link
Member

Addresses issue #2329

The xyann and anncoords properties (if people think that is a good thing to rename them to) should probably be pushed up to the _AnnotationBase level.

@tacaswell
Copy link
Member Author

One test failed on 3.3 and I think it is unrelated to these changes. Everything else passed.

@tacaswell
Copy link
Member Author

@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)
Copy link
Member

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.

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 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.

@mdboom
Copy link
Member

mdboom commented Sep 4, 2013

This looks fine (other than my style comments). It will need a test, and an entry in api_changes.rst.

@tacaswell
Copy link
Member Author

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?

@mdboom
Copy link
Member

mdboom commented Sep 5, 2013

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 @deprecated decorator makes this somewhat easy.

@tacaswell
Copy link
Member Author

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?

@tacaswell
Copy link
Member Author

Travis failures are un-related to this (tight bounding box on 2.7 and a strange syntax error on 3.2).

@tacaswell
Copy link
Member Author

Does this need anything else to get merged?

@mdboom
Copy link
Member

mdboom commented Sep 10, 2013

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 via
transforms 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-compatibility

added properties to replace xytext and textcoords (xyann, anncoords)

properly deprecated `textcoord` and `xytext` in favor of `anncoords`
and `xyann`.
@tacaswell
Copy link
Member Author

re-based

I squashed some of the commits.

mdboom added a commit that referenced this pull request Sep 10, 2013
@mdboom mdboom merged commit da6c6b5 into matplotlib:master Sep 10, 2013
@tacaswell tacaswell deleted the annotation_refactor branch September 11, 2013 15:16
tacaswell added a commit to tacaswell/matplotlib that referenced this pull request Jan 10, 2015
tacaswell added a commit to tacaswell/matplotlib that referenced this pull request Jan 10, 2015
tacaswell added a commit to tacaswell/matplotlib that referenced this pull request Jan 22, 2015
tacaswell added a commit to tacaswell/matplotlib that referenced this pull request Jul 31, 2015
…rds"

This reverts commit d6e1577, reversing
changes made to b280f7d.

For 1.4.0 tacaswell refactored annotations (matplotlib#2351 ) but missed that
{'axes points', 'axes pixel', 'figure points', 'figure pixel'} were
special cased to wrap. Presumably from the function name to maintain
back compatibility. This was an unintentional and undocumented API
break.

This API break was noticed in matplotlib#4012 and fixed in matplotlib#4019 but that catches
too many of the coordinate systems (should not be all things that start
with '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 in matplotlib#4292 where the consensus was to go with
option 1, hence this reversion.
tacaswell added a commit to tacaswell/matplotlib that referenced this pull request Jul 31, 2015
This is codifying the API change that went it in matplotlib#2351
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.

None yet

2 participants