Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Shorten implementation of Axes methods that just wrap Axis methods.#16903
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
de876cc to61c99b6CompareUh oh!
There was an error while loading.Please reload this page.
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 like this. It also helps make sure that pass-throughs remain pass-throughs
Makes the judgement in#17028 to do it on the Axis even more persuasive.
61c99b6 to98534b6CompareI pushed a slightly different implementation (directly lifting the "Placeholder" class out) and left some comments explaining why this relies on |
Uh oh!
There was an error while loading.Please reload this page.
| to the ``get_bar`` method of the ``xaxis`` attribute, and gets its | ||
| signature and docstring from ``Axis.get_bar``. | ||
| The docstring of ``get_foo`` is built by replacing "this Axis" by "the |
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'm slightly worried that subsequent edits on the docstring will break this clever replacements. People usually won't know that "this Axis" has a special meaning. They could easily write:
this axisthis `.Axis`the axis
I'm afraid that we can't do much in terms of documenting this for docstring authors or warning upon processing. But let's at least include the first two variants for replacement.
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'd rather keep things consistent. Note that there is an assertion check that the correct substring is present, with a hopefully clear error message -- and the check occurs at import time, so we'd easily catch it in CI.
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.
Ok, I did not realize the assert.
98534b6 toaab2b3fCompare| to the ``get_bar`` method of the ``xaxis`` attribute, and gets its | ||
| signature and docstring from ``Axis.get_bar``. | ||
| The docstring of ``get_foo`` is built by replacing "this Axis" by "the |
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.
Ok, I did not realize the assert.
timhoffm commentedApr 5, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Lot's of warnings in the sphinx build I think they are unrelated, but not sure about it. Edit: Same warnings in#17021. Either we have commited something that breaks these or it's caused by sphinx 3.0.0, which has been released a couple of hours ago. |
anntzer commentedApr 5, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I'd believe it's something to do with sphinx 3.0, but I'm fine waiting for that to be handled first (e.g. by temporarily pinning to <3.0) before merging this. Edit: rebased over the sphinx fix. |
... by autogenerating wrappers that forward to the Axis methods.Admittedly docstring search/replace is a bit ugly, but this has to becompared with having 4 copies of the docstrings for each method (Axis,xaxis, yaxis, zaxis) each to be kept in sync.
aab2b3f to1f62d29Compare
... by autogenerating wrappers that forward to the Axis methods.
Admittedly docstring search/replace is a bit ugly, but this has to be
compared with having 4 copies of the docstrings for each method (Axis,
xaxis, yaxis, zaxis) each to be kept in sync.
(As an anecdote, the ability to do this templatization for mplot3d was what originally motivated me to check whether minor ticks could easily be fixed for 3d axes...)
PR Summary
PR Checklist