Add helper function to check that an argument is in a list of strings. #12232
Conversation
lib/matplotlib/cbook/__init__.py
Outdated
| @@ -2075,3 +2081,14 @@ def _check_and_log_subprocess(command, logger, **kwargs): | |||
| .format(command, exc.output.decode('utf-8'))) | |||
| logger.debug(report) | |||
| return report | |||
|
|
|||
|
|
|||
| def _check_enum(values, **kwargs): | |||
There was a problem hiding this comment.
Not quite sure about the function name. This is not talking to me what it's doing. Maybe _check_contains() or _check_in_list()?
There was a problem hiding this comment.
It checks that it's a valid enumeration value, though I agree it's not a great name.
check_in_list looks backwards (btw, the reason for the **kwargs API is not to be able to squeeze multiple checks in a single call, but because that looks less bad (IMO) than _check_enum("foo", foo, [...])) and check_contains doesn't really convey the right idea to me (yes, strictly speaking, it's a containment check, but I think of it more as a validity check).
Anyways I don't feel very strongly about it but I'll wait until another reviewer chimes in so that we can agree on a name -- even though it's just a sed replace, I also need to realign the line continuations under them and I'm too lazy to do this twice.
There was a problem hiding this comment.
We have to bring in the name, the values and the allowed values somehow. I don't really have a good solution either. So just brainstorming:
Starting from:
cbook._check_enum(['tick1', 'tick2', 'grid'], which=which)
It get's a bit better with kwargs, but also more verbose:
cbook._check_param('which', value=which, allowed_values=['tick1', 'tick2', 'grid'])
rcsetup.py does something similar with ValidateInStrings:
ValidateInStrings('which', ['tick1', 'tick2', 'grid'])(which)
Similarly, one could some factory function:
cbook._arg('which', ['tick1', 'tick2', 'grid']).validate(which)
# or
cbook._checker('which', ['tick1', 'tick2', 'grid']).check(which)
That's a bit more clear with respect to the actual action and it's target, at the cost of a slightly more complex implementation.
Alternatively one could define a namedtuple('Param', 'name supported_vals') and
cbook._check_param(which, Param('which', ['tick1', 'tick2', 'grid']))
There was a problem hiding this comment.
If we're really going that route, I'd rather keep the argument and its name together (_checker([...])("foo", foo)) but that's quite close to overengineering it IMO.
There was a problem hiding this comment.
Well, maybe _check_enum("foo", foo, [...]) isn't too bad after all.
9d5bc54 to
0ab7dab
Compare
|
I'm happy with |
|
Not a fan of |
|
Thoughts about the signature as well? |
|
Renamed to _check_in_list, and rebased. |
| """ | ||
| For each *key, value* pair in *kwargs*, check that *value* is in *values*; | ||
| if not, raise an appropriate ValueError. | ||
| """ |
There was a problem hiding this comment.
Even though this is just internal, I would like an explanatory example in the docstring because the kwarg usage is a bit special. Maybe even with a note that you'll usually do _check_in_list(values, param=param).
|
Done. Also applied _check_in_list to a bunch of places in lines.py, which I had missed before. |
| if s not in self.validJoin: | ||
| raise ValueError('set_dash_joinstyle passed "%s";\n' % (s,) | ||
| + 'valid joinstyles are %s' % (self.validJoin,)) | ||
| cbook._check_in_list(self.validJoin, s=s) |
There was a problem hiding this comment.
In cases below you don't use the parameter name but the function name, e.g.
def set_default_alignment(self, d):
if d not in ["left", "right", "top", "bottom"]:
cbook._check_in_list(["left", "right", "top", "bottom"],
raise ValueError(
default_alignment=d)
default_alignment=d vs. s=s. Not terribly important but it would be good to decide on one way or the other and stick to it.
There was a problem hiding this comment.
I decided to always go with the parameter name. (Neither solution is always great, but heh, you can see the function name in the traceback anyways.)
|
Force-pushed with an additional use in parasite_axes. |
|
rebased |
|
Something went wrong ... Please have a look at my logs. |
…gument is in a list of strings.
…232-on-v3.1.x Backport PR #12232 on branch v3.1.x (Add helper function to check that an argument is in a list of strings.)
PR Summary
PR Checklist