-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Start replacing ACCEPTS table by parsing numpydoc. #11300
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
Conversation
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.
Would have merged if you didn't throw the artist.py
change in there 😉
Well I did need to change artist.py to update the regex matching... (I didn't need to update the docstring, though, indeed.) |
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.
Can you please add a test for added logic.
Parameters | ||
---------- | ||
labels : list of str | ||
labels : List[str] |
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.
Did we somewhere agree to switch to type annotation syntax?
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.
Grepping for 'List[' shows that it's definitely already being used.
lib/matplotlib/contour.py
576: lw = self.get_label_width(self.labelLevelList[lmin],
577: self.labelFmt, self.labelFontSizeList[lmin])
591: self.add_label(xmin, ymin, rotation, self.labelLevelList[lmin],
592: self.labelCValueList[lmin])
1039: artists : List[`.Artist`]
1042: labels : List[str]
1810: hatches : List[str], optional
lib/matplotlib/collections.py
386: urls : List[str] or None
387: .. ACCEPTS: List[str] or None
lib/matplotlib/tight_layout.py
52: num1num2_list : List[int]
lib/matplotlib/widgets.py
515: labels : List[str]
518: actives : List[bool], optional
2484: button : List[Int], optional
lib/matplotlib/backend_bases.py
1257: callbacks : List[Tuple[callable, Tuple, Dict]]
2368: callbacks : List[Tuple[callable, Tuple, Dict]]
lib/matplotlib/axes/_base.py
1613: v : List[float] or one of the strings listed 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.
I'll accept this for now, even tough I would like to have it discussed and agreed upon. I'm not sure if this is really the best way. Whatever the policy is, it needs documentation. I'll leave the topic for #10225.
In most cases, we can just look for 'param : type' in the docstring. We let ACCEPTS still have the priority to handle weird cases such as set_xlim where we document the first arg as "bottom", but it's "bottom, top" for the purpose of the ACCEPTS table; resolving *that* problem is punted to later. Only removed a few ACCEPTS entries as a proof of principle.
Added test. |
Parameters | ||
---------- | ||
labels : list of str | ||
labels : List[str] |
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'll accept this for now, even tough I would like to have it discussed and agreed upon. I'm not sure if this is really the best way. Whatever the policy is, it needs documentation. I'll leave the topic for #10225.
# Much faster than list(inspect.signature(func).parameters)[1], | ||
# although barely relevant wrt. matplotlib's total import time. | ||
param_name = func.__code__.co_varnames[1] | ||
match = re.search("(?m)^ *{} : (.+)".format(param_name), docstring) |
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 not quite sure, what we want the exact sematics to be.
What about foo : int, optional, default: None
?
Do we want just int
or int, optional, default: None
?
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.
These are all setters, so the first argument "should" never be optional or have a default. As always, there are exceptions (set_xlim(left=...)
), but again the point is to handle these with the fallback ACCEPTS table.
I don't think it's worth adding more machinery to this.
:class:`Artists` are of the same type) and it is your responsibility | ||
to make sure this is so. | ||
Initialize the artist inspector with an `Artist` or an iterable of | ||
`Artist`\s. If an iterable is used, we assume it is a homogeneous |
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.
This make the tests fail for me: SyntaxError: invalid escape sequence \s
Do I have an old version of something lying around?
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.
Handled by #11336
In most cases, we can just look for 'param : type' in the docstring.
We let ACCEPTS still have the priority to handle weird cases such as
set_xlim where we document the first arg as "bottom", but it's "bottom,
top" for the purpose of the ACCEPTS table; resolving that problem is
punted to later.
Only removed a few ACCEPTS entries as a proof of principle.
PR Summary
PR Checklist