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

Set up `type_checked` tag for internal type hint migration #7886

Merged
merged 2 commits into from Jun 17, 2019

Conversation

3 participants
@Eric-Arellano
Copy link
Contributor

commented Jun 14, 2019

Problem

Per #6742, we will be incrementally adding type hints to Pants.

We need some way to mark to our CI that the target should be checked against, to make sure that any gains we make in type hints are actually enforced.

Solution

Introduce a new tag type_checked, which indicates to our CI that ./pants mypy should be ran against that target.

Also introduce a helper script build-support/bin/mypy.py that runs ./pants mypy over all eligible targets in Pants. To run over a select few targets, users should instead simply run ./pants mypy path/to:target.

Alternative solution considered: centralized whitelist file

It was decided that a decentralized system will work better in this particular case, for some of these reasons:

  1. There's a convention for keeping it decentralized: the integration tag.
  2. BUILD files are more accessible to modify, e.g. being closer to the source.
  3. We want to type check the entire codebase eventually. One centralized file would risk becoming too large and difficult to maintain.

@Eric-Arellano Eric-Arellano requested review from ity, codealchemy and blorente Jun 14, 2019

@Eric-Arellano Eric-Arellano added this to In Progress in Python 3 post-migration code improvements via automation Jun 14, 2019

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2019

Alternative tag name we could use: typed.

@OniOni liked both typed and type_checked. We like typed for being more terse, but Mathieu pointed out that typed can be a bit vague and type_checked is more clear.

Let us know which you prefer!

@codealchemy
Copy link
Contributor

left a comment

👍

Show resolved Hide resolved build-support/bin/mypy.py Outdated
@blorente
Copy link
Contributor

left a comment

Looks great!

@Eric-Arellano Eric-Arellano merged commit c488456 into pantsbuild:master Jun 17, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Python 3 post-migration code improvements automation moved this from In Progress to Done Jun 17, 2019

@Eric-Arellano Eric-Arellano deleted the Eric-Arellano:type-checked-tag branch Jun 17, 2019

cattibrie added a commit to cattibrie/pants that referenced this pull request Jun 19, 2019

Set up `type_checked` tag for internal type hint migration (pantsbuil…
…d#7886)

### Problem
Per pantsbuild#6742, we will be _incrementally_ adding type hints to Pants.

We need some way to mark to our CI that the target should be checked against, to make sure that any gains we make in type hints are actually enforced.

### Solution
Introduce a new tag `type_checked`, which indicates to our CI that `./pants mypy` should be ran against that target.

Also introduce a helper script `build-support/bin/mypy.py` that runs `./pants mypy` over _all_ eligible targets in Pants. To run over a select few targets, users should instead simply run `./pants mypy path/to:target`.

#### Alternative solution considered: centralized whitelist file
It was decided that a decentralized system will work better in this particular case, for some of these reasons:

1) There's a convention for keeping it decentralized: the `integration` tag.
2) `BUILD` files are more accessible to modify, e.g. being closer to the source.
3) We want to type check the entire codebase eventually. One centralized file would risk becoming too large and difficult to maintain.
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.