Skip to content

Rename some helper classes in dviread. #9847

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

Closed

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Nov 24, 2017

Currently, the existence of matplotlib.dviread.Text prevents one to
write \~.Text`in the docs when referring tomatplotlib.text.Text`
because sphinx does not know which of the two classes we are referring
to. (It seems clear to favor matplotlib.text.Text which is actually public
API.)

Rename matplotlib.dviread.Text to _Text, as there isn't (AFAICT) any
reason to use it externally. Also renamed _Page and _Box for
consistency.

It turns out that the class deprecation machinery does not work for
deprecating namedtuples, but attaching the deprecation warning to
__new__ instead of __init__ fixes that.

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

Currently, the existence of `matplotlib.dviread.Text` prevents one to
write `\`~.Text\`` in the docs when referring to `matplotlib.text.Text`
because sphinx does not know which of the two classes we are referring
to.

Rename `matplotlib.dviread.Text` to `_Text`, as there isn't (AFAICT) any
reason to use it externally.  Also renamed `_Page` and `_Box` for
consistency.

It turns out that the class deprecation machinery does not work for
deprecating namedtuples, but attaching the deprecation warning to
`__new__` instead of `__init__` fixes that.
@anntzer anntzer force-pushed the rename-dviread-internal-classes branch from ba4a9eb to c1e03e6 Compare November 24, 2017 09:11
Copy link
Member

@jklymak jklymak left a comment

Choose a reason for hiding this comment

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

This makes sense to me. If there are no tests, then I'd assume people don't use these externally (right??)

@tacaswell
Copy link
Member

These are the return types from public functions, I am 👎 on making them private. Tacking 'DVI' on to the front or (if it makes sphinx happy) making the classes attributes on the DVI class are better options.

In any case this will require docs.

Copy link
Member

@tacaswell tacaswell left a comment

Choose a reason for hiding this comment

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

👎 to making return types from public methods (iteration on DVI) private classes.

Either give a prefix or move inside of DVI

@tacaswell tacaswell added this to the v2.2 milestone Nov 25, 2017
@anntzer
Copy link
Contributor Author

anntzer commented Nov 28, 2017

Overridden by #9854.

@anntzer anntzer closed this Nov 28, 2017
@anntzer anntzer deleted the rename-dviread-internal-classes branch November 28, 2017 02:37
@QuLogic QuLogic removed this from the v2.2 milestone Nov 28, 2017
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.

4 participants