-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Add helper function to check that an argument is in a list of strings. #12232
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/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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
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.
Looks good overall. The function naming is not perfect, but as discussed we don't have a better name.
""" | ||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
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.
Seems fine subject to rebase and CI passing
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