-
-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
Fix a bunch of doc/comment typos in patches.py. #11299
Conversation
@@ -1906,6 +1905,13 @@ def register(klass, name, style): | |||
klass._style_list[name] = style | |||
|
|||
|
|||
def _register_style(style_list, cls=None, *, name=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.
This needs a comment describing what it does and why.
The list being filled before was called EDIT: OK, I see, _style_list
. I'm not sure what that list does, but you've dropped the underscore below.style_list
is the argument. Sorry. But then really not sure what this gets us since we have to pass the list in as an argument.
Is there an actual advantage to having this a decorator versus filling the global?
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.
In my view it's more or less the same reason why
@property
def foo(self): ...
is better than
def foo(self): ...
foo = property(foo)
i.e. you immediately declare one of the most important things you're going to do with the newly declared function/class (in our cases, stash it in the style dict).
Added a 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 always uncertain if a registering decorator a good idea. It makes the registration more intransparent. On the other hand it can be more semantic. In the present case I tend to think it's an improvement.
What I don't like is the _style_list
param. At best we could get rid of it. However, I don't think that's possible (there is no such thing a decorator owned by the class, that can access the class variable _style_list
. At least, can we rename the _style_list
class variables to _styles
or even better _style_classes
.
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, oh. style_list
isn't even a list. 👿
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.
Re: the param: yes, we could do some stack walking, but that seems totally overkill.
While _style_list is indeed a terrible name, and technically private, it is actually exposed in the custom_boxstyle02.py example, and should be cleaned up from there first (as in, we either need to not support custom styles, or have a public API to do so), which I'll leave to a separate PR.
lib/matplotlib/patches.py
Outdated
The coordinates of the vertices as a Nx2 | ||
ndarray or iterable of pairs. | ||
xy : Nx2 array-like | ||
The coordinates of the vertices as a Nx2 array-like. |
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.
could leave out "as a ..." (already given in the type).
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.
done
@@ -1906,6 +1905,13 @@ def register(klass, name, style): | |||
klass._style_list[name] = style | |||
|
|||
|
|||
def _register_style(style_list, cls=None, *, name=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.
I'm always uncertain if a registering decorator a good idea. It makes the registration more intransparent. On the other hand it can be more semantic. In the present case I tend to think it's an improvement.
What I don't like is the _style_list
param. At best we could get rid of it. However, I don't think that's possible (there is no such thing a decorator owned by the class, that can access the class variable _style_list
. At least, can we rename the _style_list
class variables to _styles
or even better _style_classes
.
Also some small code cleanups there. The alias getters are already defined by cbook._define_aliases.
Undeprecate Polygon.xy from #11299
Also some small code cleanups there. The alias getters are already
defined by cbook._define_aliases.
PR Summary
PR Checklist