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

use `type=` to register enum options #7304

Merged
Merged
Changes from 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

convert register_enum_option() to .register_option() and add `idempot…

…ent_type_check` to register()
  • Loading branch information...
cosmicexplorer committed Mar 2, 2019
commit 29397addb14736a59ff3ec156e320fb68f37ab1d
@@ -15,7 +15,7 @@
from pants.process.subprocess import Subprocess
from pants.task.task import Task, TaskBase
from pants.util.memo import memoized_property
from pants.util.objects import enum, register_enum_option
from pants.util.objects import enum


class NailgunTaskBase(JvmToolTaskMixin, TaskBase):
@@ -30,8 +30,8 @@ class ExecutionStrategy(enum([NAILGUN, SUBPROCESS, HERMETIC])): pass
@classmethod
def register_options(cls, register):
super(NailgunTaskBase, cls).register_options(register)
register_enum_option(
register, cls.ExecutionStrategy, '--execution-strategy', default=cls.NAILGUN,
cls.ExecutionStrategy.register_option(
register, '--execution-strategy', default=cls.NAILGUN,
help='If set to nailgun, nailgun will be enabled and repeated invocations of this '
'task will be quicker. If set to subprocess, then the task will be run without nailgun. '
'Hermetic execution is an experimental subprocess execution framework.')
@@ -49,10 +49,7 @@ def register_options(cls, register):

@memoized_property
def execution_strategy_enum(self):
# TODO: This .create() call can be removed when the enum interface is more stable as the option
# is converted into an instance of self.ExecutionStrategy via the `type` argument through
# register_enum_option().
return self.ExecutionStrategy(self.get_options().execution_strategy)
return self.get_options().execution_strategy

@classmethod
def subsystem_dependencies(cls):
@@ -12,7 +12,7 @@
from pants.subsystem.subsystem import Subsystem
from pants.util.memo import memoized_property
from pants.util.meta import classproperty
from pants.util.objects import enum, register_enum_option
from pants.util.objects import enum


class ToolchainVariant(enum(['gnu', 'llvm'])): pass
@@ -37,8 +37,8 @@ def register_options(cls, register):
help='The default for the "compiler_option_sets" argument '
'for targets of this language.')

register_enum_option(
register, ToolchainVariant, '--toolchain-variant', advanced=True, default='gnu',
ToolchainVariant.register_option(
This conversation was marked as resolved by cosmicexplorer

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Mar 2, 2019

Contributor

This works, but I think it's seen as better style to do cls.register_option, because it makes it more immediately clear this method belongs to this class rather than another class.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 2, 2019

Author Contributor

I have changed all of these to normal register() calls!

register, '--toolchain-variant', advanced=True, default='gnu',
help="Whether to use gcc (gnu) or clang (llvm) to compile C and C++. Currently all "
"linking is done with binutils ld on Linux, and the XCode CLI Tools on MacOS.")

@@ -263,9 +263,7 @@ def setup_legacy_graph(native, options_bootstrapper, build_configuration):
options_bootstrapper,
build_configuration,
native=native,
# TODO(#7233): convert this into an enum instance using the `type` argument in option
# registration!
glob_match_error_behavior=GlobMatchErrorBehavior(bootstrap_options.glob_expansion_failure),
glob_match_error_behavior=bootstrap_options.glob_expansion_failure,
build_ignore_patterns=bootstrap_options.build_ignore,
exclude_target_regexps=bootstrap_options.exclude_target_regexp,
subproject_roots=bootstrap_options.subproject_roots,
@@ -16,7 +16,7 @@
from pants.option.optionable import Optionable
from pants.option.scope import ScopeInfo
from pants.subsystem.subsystem_client_mixin import SubsystemClientMixin
from pants.util.objects import datatype, enum, register_enum_option
from pants.util.objects import datatype, enum


class GlobMatchErrorBehavior(enum(['ignore', 'warn', 'error'])):
@@ -195,10 +195,8 @@ def register_bootstrap_options(cls, register):
help='Paths to ignore for all filesystem operations performed by pants '
'(e.g. BUILD file scanning, glob matching, etc). '
'Patterns use the gitignore syntax (https://git-scm.com/docs/gitignore).')
register_enum_option(
# TODO: allow using the attribute `GlobMatchErrorBehavior.warn` for more safety!
register, GlobMatchErrorBehavior, '--glob-expansion-failure', default='warn',
advanced=True,
GlobMatchErrorBehavior.register_option(
This conversation was marked as resolved by cosmicexplorer

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Mar 2, 2019

Contributor

Ditto on using cls.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 2, 2019

Author Contributor

I have changed all of these to normal register() calls!

register, '--glob-expansion-failure', default='warn', advanced=True,
help="Raise an exception if any targets declaring source files "
"fail to match any glob provided in the 'sources' argument.")

@@ -341,7 +341,7 @@ def _check_deprecated(self, dest, kwargs):
'type', 'member_type', 'choices', 'dest', 'default', 'implicit_value', 'metavar',
'help', 'advanced', 'recursive', 'recursive_root', 'registering_class',
'fingerprint', 'removal_version', 'removal_hint', 'fromfile', 'mutually_exclusive_group',
'daemon'
'daemon', 'idempotent_type_check',
}

# TODO: Remove dict_option from here after deprecation is complete.
@@ -442,7 +442,15 @@ def to_value_type(val_str):
elif kwargs.get('type') == bool:
return self._ensure_bool(val_str)
else:
return self._wrap_type(kwargs.get('type', str))(val_str)
type_arg = kwargs.get('type', str)
This conversation was marked as resolved by cosmicexplorer

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Mar 2, 2019

Contributor

File needs a from builtins import str import now.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 2, 2019

Author Contributor

Done!

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 3, 2019

Author Contributor

Done!

if (not kwargs.get('idempotent_type_check', True)) and isinstance(val_str, type_arg):
This conversation was marked as resolved by cosmicexplorer

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Mar 2, 2019

Contributor

Parentheses are redundant.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 2, 2019

Author Contributor

This conditional was removed!

return val_str
else:
try:
return self._wrap_type(type_arg)(val_str)
This conversation was marked as resolved by cosmicexplorer

This comment has been minimized.

Copy link
@benjyw

benjyw Mar 2, 2019

Contributor

Why do we need idempotent_type_check? Why not always convert?

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 2, 2019

Author Contributor

Because after #7269, enum classes don't have a .create() classmethod which accepts instances of the enum as well as valid values for the enum (i.e. the enum constructor will raise if provided an instance of itself). Given the rationale in this file (that all the other types are idempotent, e.g. str(str(x)) is the same as str(x)), the better move here seems to be to remove the idempotent_type_check registration option and this conditional here, and to add a check in the __new__ method for enums to allow passing in an instance of the enum class, or a valid value for the enum.

cc @illicitonion ^ I can't immediately think of a cleaner alternative than the above.

This comment has been minimized.

Copy link
@benjyw

benjyw Mar 2, 2019

Contributor

Yeah, it does seem reasonable that enum values should be just be idempotent.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 2, 2019

Author Contributor

Done!

except TypeError as e:
raise ParseError('Error applying types to option values for option {} in {}: {}'
This conversation was marked as resolved by cosmicexplorer

This comment has been minimized.

Copy link
@benjyw

benjyw Mar 2, 2019

Contributor

This error can mention the type and the value, no?

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 2, 2019

Author Contributor

Yes! Will do.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 2, 2019

Author Contributor

Done, and modified testing to match!

.format(dest, self._scope_str(), e))

# Helper function to expand a fromfile=True value string, if needed.
def expand(val_str):
@@ -517,6 +525,8 @@ def expand(val_str):
# Rank all available values.
# Note that some of these values may already be of the value type, but type conversion
# is idempotent, so this is OK.
# NB: `idempotent_type_check=False` allows type conversions which are not idempotent (such as
# enums) by checking if values are instances of the value type.

values_to_rank = [to_value_type(x) for x in
[flag_val, env_val_str, config_val_str,
@@ -289,6 +289,14 @@ def __new__(cls, value):
# won't call this __eq__ (and therefore won't raise like we want).
def __eq__(self, other):
"""Redefine equality to avoid accidentally comparing against a non-enum."""
# Options parsing will use == to check whether an option's value has changed when recording
# where the option's value was set (in pants.ini, the command line, the environment). If the
# option isn't set and falls back to the default, it will be compared against None. Limiting
# __eq__ to raise on some comparisons is intended to avoid accidental comparisons against
# e.g. a string not wrapped in an enum instance -- comparing to None is much less confusing
# and not clearly an error, so we just return False here.
This conversation was marked as resolved by cosmicexplorer

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Mar 2, 2019

Contributor

I think I see what you're trying to say, but got a bit confused between the None part and the raise part. Consider tweaking the sentence order to this:

# Options parsing will use == to check whether an option's value has changed when recording
# where the option's value was set (in pants.ini, the command line, the environment). It raises on certain comparisons to avoid accidental comparisons, e.g. a string not being wrapped in an enum instance.
# Note if the option isn't set and it falls back to the default, then the comparison will be against `None`. Comparing # to None is not clearly an error, so we just return False here.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 2, 2019

Author Contributor

I've modified it to now read:

      # This __eq__ implementation raises on anything that's not an instance of this enum class to
      # avoid accidental comparisons, e.g. a string not being wrapped in an enum instance. However,
      # options parsing will use == to check whether an option's value has changed when recording
      # where the option's value was set (in pants.ini, the command line, the environment). If the
      # option isn't set and it falls back to the default, then there will be a comparison against
      # None. Comparing an enum to None is not clearly an error, so we just return False here.

Let me know if that's more clear!

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Mar 3, 2019

Contributor

Much better! The However still confuses me though, as will use == seems like the normal default case and I would expect however to be setting up you describing some special case. I'd remove the however and say something like If the types are valid, options parsing will use ==.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 3, 2019

Author Contributor

(I spent a few minutes debating whether to add the "however" -- I can take it out again!)

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 3, 2019

Author Contributor

I keep going back and forth here -- turned it into:

Currently, options parsing will use ==

This comment has been minimized.

Copy link
@benjyw

benjyw Mar 3, 2019

Contributor

Do you even need this comment at all? It seems like a lot of verbiage in order to justify something that doesn't need justification. Allowing comparisons to None is an extremely reasonable thing to do.

In fact, it's ~in violation of the contract of __eq__ to raise an exception at all. I think you're supposed to return the NotImplemented singleton. But that's another issue, I'm fine with raising for extra caution, but that's the only thing that needs explaining, not why you allow None.

My issue with comments like this is that they reference implementation details of faraway code, and will not get updated when that faraway code changes. So better to just be rid of it entirely. No one will think it's odd that you allow comparisons to None.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 3, 2019

Author Contributor

This sounds great -- I will remove the copious verbiage and add a note about looking into better conforming to the __eq__ contract as well.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 3, 2019

Author Contributor

Done!

if other is None:
return False
if type(self) != type(other):
raise self.make_type_error(
"when comparing {!r} against {!r} with type '{}': "
@@ -329,6 +337,16 @@ def iterate_enum_variants(cls):
"""
return cls._singletons.values()

@classmethod
def register_option(cls, register, *args, **kwargs):
This conversation was marked as resolved by cosmicexplorer

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Mar 2, 2019

Contributor

Can any of the args be specified explicitly, like idempotent_type_check? *args, **kwargs params are quite difficult to understand what is expected and what is not valid. It seems we may need to keep it, but even if we could specify one or two more arguments explicitly that would be a win.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 2, 2019

Author Contributor

Do you mean requiring the caller to explicitly specify those arguments when registering an enum option? After some thought that is I think what I arrived at in https://github.com/pantsbuild/pants/pull/7304/files/c4ca5d5134c6d15411b1f67b6c202fe77a977d45#r261845170, which would remove this method altogether for the reasons you've described.

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Mar 3, 2019

Contributor

No, what I meant is in this function definition making it explicit what the parameters are. The callers can still avoid having to specify every argument by giving default arguments to the function definition.

For example

def register_option(cls, register, is_idempotent, *args, **kwargs):
  pass

Is better and more explicit than

def register_option(cls, register, *args, **kwargs):
  pass

Sounds like this is resolved either way.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 3, 2019

Author Contributor

Oh -- I get that now. This bullet was dodged but will keep this in mind for the future.

This conversation was marked as resolved by cosmicexplorer

This comment has been minimized.

Copy link
@benjyw

benjyw Mar 2, 2019

Contributor

It seems like

register(type=MyEnum, ...)

would be more consistent, and thus clearer, than

MyEnum.register_option(...I have to pass in register, but no type here, weird...)

Also this creates a "moral circular dependency" between this class and the options package. There's no reason this code should "know" anything about options, even if technically it doesn't introduce a build graph dependency.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 2, 2019

Author Contributor

I agree, in #7233 I was thinking we would instead add the ability to extract choices from a type automagically, and thought this would be less intrusive (but I see the point that this code really shouldn't know anything about options). A simple alternative is to just require that enum options explicitly add choices=MyEnumType.iterate_enum_variants() during registration, but it might be nice to specify that if the value of a type argument to register() has some attribute (e.g. choices), that the choices could be extracted automatically.

I think the right way to go in this PR (echoing @Eric-Arellano above as well) might be to use the normal register() method, and just require explicitly passing choices=MyEnumType.iterate_enum_variants() for now, and focus this PR on figuring out the idempotent_type_check issue.

This comment has been minimized.

Copy link
@benjyw

benjyw Mar 2, 2019

Contributor

Why not have the options registrar do the right thing (i.e., set choices) when it notices that the type is an enum?

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 2, 2019

Author Contributor

I have modified the check() method defined in Parser#_compute_value() to use the result of .iterate_enum_values() for choices if there is a type arg that has that attribute!

"""Register an option which converts the given value into the specified enum."""
# Since __new__ will raise if provided an instance of the enum class (as opposed to a valid
# /value/ for the enum), `idempotent_type_check=False` avoids wrapping any instances of this
# enum type again if they are already instances of this enum type.
register(*args, idempotent_type_check=False, type=cls,
choices=list(cls.iterate_enum_variants()),
**kwargs)

# Python requires creating an explicit closure to save the value on each loop iteration.
accessor_generator = lambda case: lambda cls: cls(case)
for case in all_values_realized:
@@ -340,12 +358,6 @@ def iterate_enum_variants(cls):
return ChoiceDatatype


# TODO(#7233): allow usage of the normal register() by using an enum class as the `type` argument!
def register_enum_option(register, enum_cls, *args, **kwargs):
"""A helper method for declaring a pants option from an `enum()`."""
register(*args, choices=enum_cls._allowed_values, **kwargs)


# TODO: make these members of the `TypeConstraint` class!
class TypeConstraintError(TypeError):
"""Indicates a :class:`TypeConstraint` violation."""
@@ -35,6 +35,7 @@
from pants.util.collections import assert_single_element
from pants.util.contextutil import temporary_file, temporary_file_path
from pants.util.dirutil import safe_mkdtemp
from pants.util.objects import enum


def task(scope):
@@ -85,7 +86,11 @@ def _parse(self, args_str, env=None, config=None, bootstrap_option_values=None,
task('scoped.a.bit'),
task('scoped.and-dashed'),
task('fromfile'),
task('fingerprinting')]
task('fingerprinting'),
task('enum-opt'),
task('separate-enum-opt-scope')]

class SomeEnumOption(enum(['a-value', 'another-value'])): pass

def _register(self, options):
def register_global(*args, **kwargs):
@@ -185,6 +190,16 @@ def register_global(*args, **kwargs):
options.register('fingerprinting', '--fingerprinted', fingerprint=True)
options.register('fingerprinting', '--definitely-not-fingerprinted', fingerprint=False)

# For enum tests
self.SomeEnumOption.register_option(
lambda *args, **kwargs: options.register('enum-opt', *args, **kwargs),
'--some-enum')
# For testing the default value
self.SomeEnumOption.register_option(
lambda *args, **kwargs: options.register('separate-enum-opt-scope', default='a-value',
*args, **kwargs),
'--some-enum')

def test_env_type_int(self):
options = Options.create(env={'PANTS_FOO_BAR': "['123','456']"},
config=self._create_config({}),
@@ -768,9 +783,11 @@ def check_scoped_spam(scope, expected_val, env):
check_scoped_spam('scoped.and-dashed', 'value', {'PANTS_SCOPED_AND_DASHED_SPAM': 'value'})

def test_drop_flag_values(self):
options = self._parse('./pants --bar-baz=fred -n33 --pants-foo=red simple -n1',
env={'PANTS_FOO': 'BAR'},
config={'simple': {'num': 42}})
options = self._parse(
'./pants --bar-baz=fred -n33 --pants-foo=red enum-opt --some-enum=another-value simple -n1',
env={'PANTS_FOO': 'BAR'},
config={'simple': {'num': 42},
'enum-opt': {'some-enum': 'a-value'}})
defaulted_only_options = options.drop_flag_values()

# No option value supplied in any form.
@@ -789,6 +806,21 @@ def test_drop_flag_values(self):
self.assertEqual('red', options.for_global_scope().pants_foo)
self.assertEqual('BAR', defaulted_only_options.for_global_scope().pants_foo)

# Overriding an enum option value.
self.assertEqual(self.SomeEnumOption.another_value, options.for_scope('enum-opt').some_enum)

# Getting the default value for an enum option.
self.assertEqual(self.SomeEnumOption.a_value,
defaulted_only_options.for_scope('separate-enum-opt-scope').some_enum)

def test_enum_option_type_parse_error(self):
with self.assertRaises(ParseError) as cm:
options = self._parse('./pants enum-opt --some-enum=invalid-value')
options.for_scope('enum-opt').some_enum
self.assertIn("""\
ParseError: Error applying types to option values for option some_enum in scope 'enum-opt': type check error in class SomeEnumOption: Value 'invalid-value' must be one of: ['a-value', 'another-value'].""",
str(cm.exception))

def test_deprecated_option_past_removal(self):
"""Ensure that expired options raise CodeRemovedError on attempted use."""
# NB: these exceptions are triggered by the `Parser#parse_args()` method, not
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.