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. |
| 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. |
anntzer
left a comment
anntzer
left a comment
There was a problem hiding this comment.
We can always roll this back when we move to Py3 only (hopefully soon).
| """ | ||
| 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. |
dstansby
force-pushed
the
unused-kwarg-warnings
branch
from
30ad984 to
4481344
Compare
|
Do we need the rename suggested here? |
dopplershift
left a comment
dopplershift
left a comment
There was a problem hiding this comment.
This improves the status quo, so let's not hold it up for theoretically better naming of old functions.
|
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. |
5 participants
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!