-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Change manual kwargs popping to kwonly arguments. #10545
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
lib/matplotlib/contour.py
Outdated
def clabel(self, *args, **kwargs): | ||
def clabel(self, *args, | ||
fontsize=None, inline=True, inline_spacing=5, fmt='%1.3f', | ||
colors=None, use_clabeltext=False, manual=False): |
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.
rightside_up missing
def print_pdf(self, filename, **kwargs): | ||
image_dpi = kwargs.get('dpi', 72) # dpi to use for images | ||
def print_pdf(self, filename, *, | ||
dpi=72, # dpi to use for images |
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.
Maybe deprecate dpi and replace by image_dpi? During the deprecation dpi could be catched via kwargs.
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.
nah, let's try to keep all the print_foo's with as much the same signature as possible
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.
@@ -790,8 +790,9 @@ class FigureCanvasPgf(FigureCanvasBase): | |||
def get_default_filetype(self): | |||
return 'pdf' | |||
|
|||
def _print_pgf_to_fh(self, fh, *args, **kwargs): | |||
if kwargs.get("dryrun", False): | |||
def _print_pgf_to_fh(self, fh, *args, |
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.
*args is unused -> * only
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.
see above re: keeping the same signature for all print_foo's. In any case this would mean fixing the call sites as well, so it'll be separate work.
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.
lib/matplotlib/tests/test_figure.py
Outdated
# whether the arguments are iterable, and if so we try and convert them | ||
# to a tuple. However, the ``iterable`` function returns True if | ||
# __getitem__ is present, but some classes can define __getitem__ without | ||
# being iterable. The tuple conversion is now done in a try...except in | ||
# case it fails. | ||
|
||
class MyAxes(Axes): | ||
def __init__(self, *args, **kwargs): | ||
kwargs.pop('myclass', None) | ||
def __init__(self, *args, myclass=None **kwargs): |
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.
missing comma
plt.draw_if_interactive() | ||
return ax | ||
|
||
def host_subplot(*args, **kwargs): | ||
def host_subplot(*args, axes_class=None, **kwargs): |
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.
missing figure kwarg
orientation=kwargs.pop("orientation", None) | ||
if orientation is None: | ||
raise ValueError("orientation must be specified") | ||
def __init__(self, *args, orientation, **kwargs): |
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.
Not exactly the same: You can now explicitly pass orientation=None and it will not raise a ValueError.
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.
orientation="foobar"
was probably also invalid to start with, so there must be a downstream check that will also prune None out.
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.
Maybe you're right. I don't follow the code path here. And I couldn't find out whose responsibility checking or orientation
really is.
When doing
ImageGrid(plt.figure(), (0, 0, 1,1), (1, 2), cbar_mode='single', cbar_location='foobar')
it finally raises a KeyError
on ax.axis[self.orientation].toggle(all=b)
in CbarAxesBase._config_axes()
. However that seems more by chance than an intended check.
Before that we have code executed like
if self.orientation in ["top", "bottom"]:
orientation = "horizontal"
else:
orientation = "vertical"
which is locally coercing all invalid values to 'vertical'.
The whole thing seems a little brittle to me. But this is beyond this PR. By me, it's ok to not reject orientation=None in this case. Some code downstream will complain.
thanks, comments handled |
2501cbf
to
006cf54
Compare
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 don't understand the Travis build failure, but it looks more like a setup issue in the build rather than an error due to this PR.
rebased |
f4078d3
to
edd44da
Compare
rebased |
I'd need more motivation to plow thorugh 37 files of changes. Whats the advantage of this PR? |
I would argue that
is a pretty big readability improvement. |
Fair, but what does this do? Why is there the extra * in there?
|
That means that
one can call One not-so-obvious advantage of using kwonly arguments is that it allows us to change the order of kwargs in the doc much more easily (see #7856 for a real-life example). |
Thanks - I'll take a quick look soon! |
Rebased. This PR caught a spurious kwarg that was added to the example call to clabel() at ec5e886#diff-6585abef3f24dfdde728a606b14e78efR93 and removes it. |
Only simple cases (no mutable defaults, no defaults depending on other args) are handled so far.
Only simple cases (no mutable defaults, no defaults depending on other
args) are handled so far.
PR Summary
PR Checklist