Warn about unused kwargs in contour methods #7728
Conversation
|
I am concerned about spending too much time on this before we drop python2. Once we are 3 only we can start to use keyword-only arguments so anything the function uses goes in the signature (no popping!) and kwargs are only used when things need to fall through to the next layer down. |
lib/matplotlib/contour.py
Outdated
| for key, value in kwargs.items(): | ||
| s += '"' + str(key) + '"' + ', ' | ||
| warnings.warn('The following kwargs were not used by contour: ' + | ||
| s) |
There was a problem hiding this comment.
Could just be ", ".join(map(repr, kwargs)).
|
Still, it looks like a reasonable improvement. Minor nitpick, but otherwise looks good. |
| @@ -1532,7 +1542,7 @@ def _check_xyz(self, args, kwargs): | |||
| Exception class (here and elsewhere). | |||
| """ | |||
| x, y = args[:2] | |||
| self.ax._process_unit_info(xdata=x, ydata=y, kwargs=kwargs) | |||
| kwargs = self.ax._process_unit_info(xdata=x, ydata=y, kwargs=kwargs) | |||
There was a problem hiding this comment.
In this case, it's passed as a mutable value, so you don't technically need to return it. It should be the same object coming out as going in.
There was a problem hiding this comment.
I would suggest renaming the method to _pop_and_process_unit_info (_pop_and_apply_unit_info to save two characters) for clarity -- sneakily modifying a mutable argument usually leads to a lot of head-scratching.
There was a problem hiding this comment.
I am 👎 on renaming it because it has had the current name for a long time and has always probably had this pop behavior.
In any case that change is outside of the scope if this PR.
|
'powercycled to re-run against current master + restart CI. |
|
circle CI is failing because it needs to be re-based on current master to pick up the proper config. |
30ad984 to
4481344
Compare
|
Do we need the rename suggested here? |
|
Note that this warning is somewhat the opposite of what's requested in #2369, i.e., pass extra keyword arguments to the artist properties of the generated contour object. |
I got interested in #1963 (and I think there's another similar bug somewhere out there...), so thought I'd have a go.
The general method behind what I've done is
poped__init__block, all the kwargs that are used have gone, and any unused kwargs are left behindI think it should be in theory fairly easy (but very tedious) to roll this out to all methods.
This is probably fairly experimental, so happy for it to be discussed/reviewed and not necessarily merged!