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

Expand type constraints allowed in datatype() #5774

Merged
merged 6 commits into from May 3, 2018

Conversation

Projects
None yet
3 participants
@cosmicexplorer
Copy link
Contributor

cosmicexplorer commented May 1, 2018

Problem

You can declare type-checked fields with an Exactly constraint on a specific type (so checking type(x) == SomeType), but you may want to just use e.g. anything that's a subclass of some base class or mixin.

Solution

  • Allow specifying a TypeConstraint in the tuple form of a field declaration for datatype(), e.g. dataype([('val', Exactly(int))]).

Result

Datatype fields no longer just have to match field types exactly -- they can now just be restricted to e.g. any subclass of some given class or mixin (with e.g. datatype([('optionable', SubclassesOf(Optionable))]).

cosmicexplorer added some commits May 1, 2018

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

cosmicexplorer commented May 3, 2018

This is ready for review.

@jsirois
Copy link
Member

jsirois left a comment

LGTM, but let me know your thoughts on these before I merge.

@@ -13,6 +13,9 @@
from pants.util.meta import AbstractClass


class DatatypeHelperConstructionError(Exception): pass

This comment has been minimized.

@jsirois

jsirois May 3, 2018

Member

The mild style bias is towards nested exception types. This allows, for example, code like:

with self.assertRaises(datatype.DatatypeHelperConstructionError):
  ...

IE: no specific import for the exception type, just a focus on importing the nominal type.

Also, since there is no DataypeHelper noun floating around, DatatypeConstructionError would make more sense to me. In any case it seems like it should extend TypeError and if that is enough distinction for tests (I think it is), you could just drop the cutom exception type altogether!

This comment has been minimized.

@cosmicexplorer

cosmicexplorer May 3, 2018

Contributor

I've noted the description of nested exception types for future reference, thanks (the imports part makes sense to me). I have removed the Helper from the exception type name, made it subclass TypeError, then caught TypeError in the test case for it.

This comment has been minimized.

@jsirois

jsirois May 3, 2018

Member

Well, if the test is checking for TypeError, there is no remaining point to a custom exception type. It's only useful to have one if someone can benefit and discriminate on the difference from a built in exception type.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer May 3, 2018

Contributor

Ok, that makes sense. I have removed the custom exception type (and changed the :raises: annotation) in f7c2565.

elif isinstance(type_spec, TypeConstraint):
type_constraint = type_spec
else:
raise DatatypeHelperConstructionError(

This comment has been minimized.

@jsirois

jsirois May 3, 2018

Member

Consider adding a :raises: ... to the function doc.



class WithSubclassTypeConstraint(datatype([('some_value', SubclassesOf(SomeBaseClass))])):
pass

This comment has been minimized.

@jsirois

jsirois May 3, 2018

Member

Pull up on previous line? That would fit in 100 cols and you place pass on the same line elsewhere, so the inconsistency draws the eye.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer May 3, 2018

Contributor

Yes, agreed and fixed. I just realized I had deleted the file with directory-local variables in the pants repo setting my line length to 100 chars, so it was defaulting to 80 (and had been for a bit). Fixed that as well on my end.

self.assertEqual(
str(some_object),
str("YetAnotherNamedTypedDatatype(a_string<=str>=asdf, an_int<=int>=45)"))
str("WithExplicitTypeConstraint(a_string<=str>=asdf, an_int<=int>=45)"))

This comment has been minimized.

@jsirois

jsirois May 3, 2018

Member

I see that there is precedent in this file, but why str("...")? This is redundant under both python2 and python3.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer May 3, 2018

Contributor

I vaguely described the reasoning I thought there might be for this, then tried just removing the str() calls, and everything worked. No clue what issue I was having before.

cosmicexplorer added some commits May 3, 2018

@jsirois

This comment has been minimized.

Copy link
Member

jsirois commented May 3, 2018

LGTM! Just one last point to address and I'm happy to merge.

@baroquebobcat
Copy link
Contributor

baroquebobcat left a comment

lgtm.

The one thought this patch brings up for me is that subclass type constraints aren't allowed in the engine right now. In some places that's explicit and in others it's implicit. Now that we can add type constraints to datatypes fields, it might make sense to think about bounding those field type constraints for types used in the engine.

I don't know what the right approach for that is though, and it's not directly related to this.

cosmicexplorer added some commits May 3, 2018

@jsirois

This comment has been minimized.

Copy link
Member

jsirois commented May 3, 2018

Thanks for iterating on this!

@jsirois jsirois merged commit 4362ec6 into pantsbuild:master May 3, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment