Skip to content
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

extend option registration to allow using enum as the option `type` #7233

Closed
cosmicexplorer opened this issue Feb 9, 2019 · 1 comment

Comments

Projects
None yet
1 participant
@cosmicexplorer
Copy link
Contributor

commented Feb 9, 2019

See #7226 (comment) -- we added register_enum_option() to make it more natural to use an enum directly as a pants option. We would like to be able to just use an enum class as the option type and remove that helper method, but there are a few things blocking that (all of which can be fixed with edits to parser.py):

  • The choices argument is applied after the type argument, and the enum constructor (which is used when the type argument is applied) checks the value and raises a TypeError. So when an option value is given that is not one of the allowed enum values, we see the enum error message (without any context) instead of the normal pants option choices error message (which says which option was invalid).
  • It would be nice to extract choices from the type argument itself. register_enum_option() currently uses the ._allowed_values classproperty in the generated enum class.
  • It would also be nice to extract default from the type argument.

I believe all of these have general utility and wouldn't increase complexity of option parsing too much.

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor Author

commented Mar 2, 2019

It would also be extremely cool to take enum cases with hyphens e.g. _JvmCompileWorkflowType('zinc-only') and convert them into _JvmCompileWorkflowType.zinc_only, very similarly to how we transform hyphenated options into underscored attributes.

cosmicexplorer added a commit that referenced this issue Mar 6, 2019

use `type=` to register enum options (#7304)
### Problem

Resolves #7233. We want to make it easy and natural to register enum options, and one way to improve that is to automatically convert options registered as enums to instances of those enums.

### Solution

- Convert hyphenated enum values to underscored `classproperty`s, e.g. `'zinc-only'` becomes `.zinc_only`.
- Remove `register_enum_option()` and use `type=` to automatically convert option values to enum instances.
- Make `__new__` for `enum` classes accept an instance of the enum class and pass it through so options parsing can assume the constructor is idempotent.
- Wrap the `type` checking area in options parsing in a `try` and convert to a `ParseError` with more information about the invalid option.
- Extract `.all_variants` from a `type` arg and use it for `choices` if not explicitly specified, allowing enum `type` options to automatically populate the `choices` argument to `register()`.
- Extract `datatype` and `enum` logic into `DatatypeMixin` (to do special-case logic in `stable_json_sha1()`) and `ChoicesMixin` (to do the special-case logic to get `choices`).

### Result

Registering options as enum classes is much more natural, and has a better error message!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.