-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
DOC: more nitpick follow up #15048
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
DOC: more nitpick follow up #15048
Conversation
The Axes classes have a multi-layered internal class structure and we generate documentation for the top-level public classes. However, we also auto-generate property tables by querying the methods on the objects which resolve to the base class they are defined on which leads to incorrect references. This is a very quick hack which special-cases two of the Axes classes and replace their name with just ``Axes``.
@ImportanceOfBeingErnest RE #15042 (comment) can you try building the docs on windows from this branch? Sometimes the path includes |
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 built the docs off this branch locally, and it does not show the error I previously got.
|
||
# sometimes the 'path' can contain ':' which are forbidden on | ||
# windows, but on posix just passes through. | ||
path, *post = path.partition(':') |
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.
uh, how can there be colons in the path, even on posix? a priori this should be a real filesystem path given that we call Path(path).resolve() just below
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.
Yes, it's confusing that this is named "path", while in reality just being some string which should contain a path at the start.
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.
As always I'm a bit confused by the magic of escaping, but :
appears to be kosher in posix paths:
sharon@21:09 ➤ touch "/tmp/path:o:log:i:cal"
sharon@09:03 ➤ echo "hi there" > "/tmp/path:o:log:i:cal"
sharon@09:03 ➤ ls /tmp/ | grep path
path:o:log:i:cal
sharon@09:03 ➤ cat /tmp/path\:o\:log\:i\:cal
hi there
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.
It's certainly kosher (anything but / and \0 are for posix), but how is that relevant here? if we're running on linux, then the path is what it is and it will become relative to the mpl root just below (in relative_to), and there are no directories in mpl with colons; if running on windows, get_source_line should be returning a real path on the windows filesystem and that has no colons.
@@ -1411,7 +1411,9 @@ def pprint_setters_rest(self, prop=None, leadingspace=4): | |||
|
|||
attrs = sorted(self._get_setters_and_targets()) | |||
|
|||
names = [self.aliased_name_rest(prop, target) | |||
names = [self.aliased_name_rest(prop, target).replace( |
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.
it doesn't have to be this PR, but perhaps we should just fix Axes.__module__
to be matplotlib.axes
, not matplotlib.axes._base
? see numpy/numpy#12382 for similar stuff on numpy's side.
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.
That seems reasonable, but also push to another PR.
This fixes an exception just merged to master and the other half of
the issues @ImportanceOfBeingErnest is having in #14917.
PR Summary
PR Checklist