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

merge pants.util.objects.datatype with python 3 dataclasses #7074

Closed
cosmicexplorer opened this issue Jan 13, 2019 · 3 comments · Fixed by #8576
Closed

merge pants.util.objects.datatype with python 3 dataclasses #7074

cosmicexplorer opened this issue Jan 13, 2019 · 3 comments · Fixed by #8576

Comments

@cosmicexplorer
Copy link
Contributor

cosmicexplorer commented Jan 13, 2019

We have a pants.util.objects.datatype type constructor, which as of #5723 can add declarative type checking to its fields upon construction. In #6374, there was a consensus that we should move to using python 3 dataclasses, but that was only available for python 3.6+ (3.6 with a polyfill), which we didn't yet support in CI. After #7078 is resolved (which blocks this), we should be able to support their use in CI.

Before that time, we should consider what datatype() provides us and what features we want or don't want from it vs python 3 dataclasses so that when the python 3 migration completes we can converge around the officially supported way of making typed named tuples.

Note that the reason we use datatype is for low-overhead hashable objects to interface with the v2 engine -- see the engine README. It's not at all ridiculous to continue to have a datatype constructor so we can maintain the qualities we need for v2 engine interaction, but it would be fantastic if we could switch the "backend" for our datatypes from namedtuple to dataclass when we complete the migration to python 3.

@cosmicexplorer
Copy link
Contributor Author

#7226 adds a TODO in objects.py to move the class method definitions into a separate mixin to ease this migration.

@cosmicexplorer
Copy link
Contributor Author

#7304 introduced DatatypeMixin to satisfy the above.

@cosmicexplorer
Copy link
Contributor Author

cosmicexplorer commented Aug 18, 2019

#8181 should introduce @dataclasstype, and after that all that remains is deprecating the namedtuple-based datatype() and runtime-checked TypeConstraint in favor of mypy.

cosmicexplorer added a commit that referenced this issue Oct 20, 2019
### Problem

A first step to fixing #7074. We would like to be able to make use of python 3 `@dataclass`es, partially because they are the canonical way to make typed tuples in python now, and partially because it allows us to use mypy to resolve type errors at "compile" time instead of performing type checks at runtime when the tuple is constructed, which is what we do now, e.g.: https://github.com/pantsbuild/pants/blob/b4e316f4badc2703cea566267886d8b094d0f569/src/python/pants/util/objects.py#L120-L125

### Solution

- Edit the engine `README.md` to demonstrate how to use `@dataclass` and remove mentions of the old `datatype()`.
- Add an integration test to verify that mypy will correctly detect type errors when constructing `@dataclass` objects.

### Result

Users are able to use a very canonical `@dataclass` declaration syntax for engine params which is statically type-checked with mypy!
Eric-Arellano added a commit that referenced this issue Nov 5, 2019
Resolves #7074. Now that we always use `@dataclass` instead of `datatype`, and replaced `Collection.of(T)` with `Collection[T]`, it is safe to remove `datatype()`.

We also remove `TypedCollection` as it is no longer in use. Instead, we use explicit type hints like `Tuple[str, ...]` or `Collection[T]`.

We avoid a deprecation cycle because V2 engine code has always been experimental.

### Keeps `util/objects.py`
We still keep `TypeConstraintError`, `Exactly`, `SuperclassesOf`, and `SubclassesOf`. These are used widely in the codebase and have no standard library equivalent. They also do not seem to cause problems for MyPy.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant