From 320de41e1880ff6e572240a193e897a2604ba0cb Mon Sep 17 00:00:00 2001 From: Danny McClanahan <1305167+cosmicexplorer@users.noreply.github.com> Date: Mon, 28 Jan 2019 00:01:38 -0800 Subject: [PATCH] Revert "do the argument checking ourselves so we can use wrapper_type" This reverts commit 5211b022bb5fe960a72a83c9b43787ee80dfdd8d. --- src/python/pants/util/objects.py | 76 ++++---------------- tests/python/pants_test/util/test_objects.py | 40 +++++------ 2 files changed, 32 insertions(+), 84 deletions(-) diff --git a/src/python/pants/util/objects.py b/src/python/pants/util/objects.py index fe2a1bd28791..6284d9b5b01a 100644 --- a/src/python/pants/util/objects.py +++ b/src/python/pants/util/objects.py @@ -66,75 +66,24 @@ def __new__(cls, *args, **kwargs): if not hasattr(cls.__eq__, '_eq_override_canary'): raise cls.make_type_error('Should not override __eq__.') - # NB: `TypeConstraint#validate_satisfied_by()` can return a different result than its input if - # a `wrapper_type` argument is provided to the base class constructor. Because `namedtuple` is - # immutable, we have to do any modifications here. The extra work in this method which - # duplicates the positional and keyword argument checking in `namedtuple` reduces a - # significant amount of boilerplate when creating `datatype` objects which accept collections, - # allowing the object's creator to pass in any type of collection as an argument and ensure - # the object is still hashable (by converting it to a tuple). We can also improve the quality - # of the argument checking error messages and ensure they are consistent across python - # versions. - if len(args) > len(field_names): - raise cls.make_type_error( - """\ -too many positional arguments: {} arguments for {} fields! -args: {} -fields: {}""" - .format(len(args), len(field_names), args, field_names)) - - # Create a dictionary of the positional and keyword arguments. - # NB: We use an OrderedDict() to ensure reproducible error messages. - arg_dict = OrderedDict() - selected_field_names = [] - for field_index, arg_value in enumerate(args): - field_name = field_names[field_index] - arg_dict[field_name] = arg_value - selected_field_names.append(field_name) - - # Raise if an argument was specified positionally and with a keyword. - overlapping_field_names = frozenset(selected_field_names) & frozenset(kwargs.keys()) - if overlapping_field_names: - raise cls.make_type_error( - """\ -arguments were specified positionally and by keyword: {}! -args: {} -kwargs: {}""".format(list(overlapping_field_names), args, kwargs)) - - # The arguments were non-overlapping, so we can safely populate the arg dict. - arg_dict.update(kwargs) - - # Check that we don't have any unknown arguments *before* we perform type checking. - unrecognized_args = frozenset(arg_dict.keys()) - frozenset(field_names) - if unrecognized_args: - raise cls.make_type_error("unrecognized arguments: {}".format(list(unrecognized_args))) - # Check that we have specified all of the non-optional arguments. - missing_args = frozenset(field_names) - frozenset(arg_dict.keys()) - if missing_args: - raise cls.make_type_error("missing arguments: {}".format(list(missing_args))) + try: + this_object = super(DataType, cls).__new__(cls, *args, **kwargs) + except TypeError as e: + raise cls.make_type_error(e) # TODO: Make this kind of exception pattern (filter for errors then display them all at once) # more ergonomic. type_failure_msgs = [] - for arg_name, arg_value in arg_dict.items(): - field_constraint = fields_with_constraints.get(arg_name, None) - if field_constraint: - try: - new_arg_val = field_constraint.validate_satisfied_by(arg_value) - except TypeConstraintError as e: - type_failure_msgs.append("field '{}' was invalid: {}".format(arg_name, e)) - new_arg_val = 'ERROR: {}'.format(e) - else: - new_arg_val = arg_value - arg_dict[arg_name] = new_arg_val + for field_name, field_constraint in fields_with_constraints.items(): + field_value = getattr(this_object, field_name) + try: + field_constraint.validate_satisfied_by(field_value) + except TypeConstraintError as e: + type_failure_msgs.append( + "field '{}' was invalid: {}".format(field_name, e)) if type_failure_msgs: raise cls.make_type_error('\n'.join(type_failure_msgs)) - try: - this_object = super(DataType, cls).__new__(cls, **arg_dict) - except TypeError as e: - raise cls.make_type_error(e) - return this_object def __eq__(self, other): @@ -354,8 +303,7 @@ def satisfied_by(self, obj): def validate_satisfied_by(self, obj): """Return some version of `obj` if the object satisfies this type constraint, or raise. - If this `TypeConstraint` instance provided a `wrapper_type` to the base class constructor, the - result will be of the type `self._wrapper_type`. + # TODO: consider disallowing overriding this too? :raises: `TypeConstraintError` if `obj` does not satisfy the constraint. """ diff --git a/tests/python/pants_test/util/test_objects.py b/tests/python/pants_test/util/test_objects.py index ecacb7e5e05d..ca8f52352796 100644 --- a/tests/python/pants_test/util/test_objects.py +++ b/tests/python/pants_test/util/test_objects.py @@ -548,42 +548,42 @@ def test_instance_with_collection_construction_str_repr(self): def test_instance_construction_errors(self): with self.assertRaises(TypeError) as cm: SomeTypedDatatype(something=3) - expected_msg = """\ -error: in constructor of type SomeTypedDatatype: type check error: -unrecognized arguments: ['something']""" + expected_msg = "error: in constructor of type SomeTypedDatatype: type check error:\n__new__() got an unexpected keyword argument 'something'" self.assertEqual(str(cm.exception), expected_msg) # not providing all the fields with self.assertRaises(TypeError) as cm: SomeTypedDatatype() - expected_msg = """\ -error: in constructor of type SomeTypedDatatype: type check error: -missing arguments: [{}'val']""".format('u' if PY2 else '') + expected_msg_ending = ( + "__new__() missing 1 required positional argument: 'val'" + if PY3 else + "__new__() takes exactly 2 arguments (1 given)" + ) + expected_msg = "error: in constructor of type SomeTypedDatatype: type check error:\n" + expected_msg_ending self.assertEqual(str(cm.exception), expected_msg) # unrecognized fields with self.assertRaises(TypeError) as cm: SomeTypedDatatype(3, 4) - expected_msg = """\ -error: in constructor of type SomeTypedDatatype: type check error: -too many positional arguments: 2 arguments for 1 fields! -args: (3, 4) -fields: [{}'val']""".format('u' if PY2 else '') + expected_msg_ending = ( + "__new__() takes 2 positional arguments but 3 were given" + if PY3 else + "__new__() takes exactly 2 arguments (3 given)" + ) + expected_msg = "error: in constructor of type SomeTypedDatatype: type check error:\n" + expected_msg_ending self.assertEqual(str(cm.exception), expected_msg) with self.assertRaises(TypedDatatypeInstanceConstructionError) as cm: CamelCaseWrapper(nonneg_int=3) - expected_msg = """\ -error: in constructor of type CamelCaseWrapper: type check error: -field 'nonneg_int' was invalid: value 3 (with type 'int') must satisfy this type constraint: Exactly(NonNegativeInt).""" + expected_msg = ( + """error: in constructor of type CamelCaseWrapper: type check error: +field 'nonneg_int' was invalid: value 3 (with type 'int') must satisfy this type constraint: Exactly(NonNegativeInt).""") self.assertEqual(str(cm.exception), expected_msg) # test that kwargs with keywords that aren't field names fail the same way with self.assertRaises(TypeError) as cm: CamelCaseWrapper(4, a=3) - expected_msg = """\ -error: in constructor of type CamelCaseWrapper: type check error: -unrecognized arguments: ['a']""" + expected_msg = "error: in constructor of type CamelCaseWrapper: type check error:\n__new__() got an unexpected keyword argument 'a'" self.assertEqual(str(cm.exception), expected_msg) def test_type_check_errors(self): @@ -678,9 +678,9 @@ def test_copy_failure(self): with self.assertRaises(TypeCheckError) as cm: obj.copy(nonexistent_field=3) - expected_msg = """\ -error: in constructor of type AnotherTypedDatatype: type check error: -unrecognized arguments: ['nonexistent_field']""" + expected_msg = ( + """error: in constructor of type AnotherTypedDatatype: type check error: +__new__() got an unexpected keyword argument 'nonexistent_field'""") self.assertEqual(str(cm.exception), expected_msg) with self.assertRaises(TypeCheckError) as cm: