Skip to content

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

Merged
merged 1 commit into from
Feb 23, 2019

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Sep 23, 2018

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@anntzer anntzer added this to the v3.1 milestone Sep 23, 2018
@@ -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):
Copy link
Member

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()?

Copy link
Contributor Author

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.

Copy link
Member

@timhoffm timhoffm Sep 23, 2018

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']))

Copy link
Contributor Author

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.

Copy link
Member

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.

@anntzer anntzer force-pushed the enumchecker branch 8 times, most recently from 9d5bc54 to 0ab7dab Compare September 24, 2018 06:03
@anntzer
Copy link
Contributor Author

anntzer commented Sep 24, 2018

I'm happy with _check_enum("foo", foo, [...]). Anyone else wants to chime in?

@jklymak
Copy link
Member

jklymak commented Oct 2, 2018

Not a fan of _check_enum; even the title of the PR is 'check that an argument is in a list of strings' so my vote is for _check_in_list.

@anntzer
Copy link
Contributor Author

anntzer commented Oct 2, 2018

Thoughts about the signature as well?

@anntzer
Copy link
Contributor Author

anntzer commented Jan 21, 2019

Renamed to _check_in_list, and rebased.

Copy link
Member

@timhoffm timhoffm left a 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.
"""
Copy link
Member

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).

@anntzer
Copy link
Contributor Author

anntzer commented Jan 21, 2019

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)
Copy link
Member

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.

Copy link
Contributor Author

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.)

@anntzer
Copy link
Contributor Author

anntzer commented Jan 22, 2019

Force-pushed with an additional use in parasite_axes.

Copy link
Member

@jklymak jklymak left a 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

@anntzer
Copy link
Contributor Author

anntzer commented Feb 22, 2019

rebased

@lumberbot-app
Copy link

lumberbot-app bot commented Feb 23, 2019

Something went wrong ... Please have a look at my logs.

meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Feb 23, 2019
@anntzer anntzer deleted the enumchecker branch February 23, 2019 08:43
dstansby added a commit that referenced this pull request Feb 23, 2019
…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.)
@anntzer anntzer mentioned this pull request Feb 27, 2019
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants