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

Setup MyPy config and enforce types for build-support scripts #7704

Merged
merged 11 commits into from May 14, 2019

Conversation

Projects
None yet
4 participants
@Eric-Arellano
Copy link
Contributor

commented May 11, 2019

Problem

We would like to use type hints throughout our code base, per #6742.

However, we must configure it with sensible defaults to make it easier to use and to provide a consistent interface for developers.

We should also enforce as much type safety as we can via our pre-commit check.

Solution

  • Add new mypy.ini file with fairly loose defaults that are good for first time codebases. Eventually, we will want to do things like require all functions to be typed, but for now we loosen our expectations.
  • Link to the config file in pants.ini, along with using the most modern MyPy release which gives us the strict_equality check.
  • Run mypy.py over the build-support scripts, which are fully typed.
    • Acts as a proof of concept for how this will be integrated into CI.

Result

./pants mypy is now a bit more useful on our codebase, even though it will still fail because of pre-existing issues.

Further, the build-support folder will not have any typing regressions now.

@Eric-Arellano Eric-Arellano referenced this pull request May 11, 2019

Open

Add type hints to codebase #6742

0 of 4 tasks complete
@blorente
Copy link
Contributor

left a comment

Cool!

Show resolved Hide resolved build-support/bin/mypy.py Outdated
"--config-file=build-support/mypy/mypy.ini",
]
try:
subprocess.run(command + targets, check=True)

This comment has been minimized.

Copy link
@blorente

blorente May 13, 2019

Contributor

Not sure which one I prefer, but iirc subprocess.check_call(command + targets) does the same thing

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano May 13, 2019

Author Contributor

In Python 3.5+ land, you almost always want to use subprocess.run(). It can handle almost every instance you can imagine, beyond async calls. I recommend skimming https://codecalamity.com/run-subprocess-run/.

You're right that subprocess.check_call() would work, but it's better for us to converge on always using subprocess.run() where possible for consistency in our codebase.

This comment has been minimized.

Copy link
@blorente

blorente May 14, 2019

Contributor

Thanks for the reference article :) I'm sure which one I prefer now, it's great as it is.

@illicitonion
Copy link
Contributor

left a comment

Awesome! Thanks!

Show resolved Hide resolved build-support/mypy/mypy.ini
@stuhood
Copy link
Member

left a comment

Even though there is already a ./pants mypy goal, we want to set up a config file with sensible defaults and to create a helper script to make it easier to run the tool. It should be extremely simple for both developers and CI to type check the code, without having to know things like where our config file is.

IMO, this definitely needs to use that task.

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2019

IMO, this definitely needs to use that task.

@stuhood miscommunication here. We are using the MyPy task. This is simply a wrapper around ./pants mypy that hooks it up to our config file. Just like how we do isort.sh, which is a wrapper around ./pants fmt.isort.

I'll rewrite the description to make this more clear.

@stuhood
Copy link
Member

left a comment

Sorry for the miscommunication... the combination of the existience of a mypy binary and the description caused me to stop short.

I'm not really sure that the mypy binary/script carries its weight. As John commented on the (related) isort review: if the output of the isort task isn't great, then we should consider improving the output of the task. And as to standardizing the options we pass: that should happen via pants.ini (see https://www.pantsbuild.org/options.html#setting-option-values).

Show resolved Hide resolved build-support/bin/mypy.py Outdated
Show resolved Hide resolved build-support/bin/mypy.py Outdated

@Eric-Arellano Eric-Arellano changed the title Setup MyPy config, add mypy.py, and enforce types for build-support scripts Setup MyPy config and enforce types for build-support scripts May 13, 2019

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

One of the shards failed due to merging 55f11c6 after the pants.pex was already built. That shard had passed on an earlier run and it is completely orthogonal to this PR, so going to merge.

@Eric-Arellano Eric-Arellano merged commit 501f953 into pantsbuild:master May 14, 2019

1 check failed

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

@Eric-Arellano Eric-Arellano deleted the Eric-Arellano:setup-mypy branch May 14, 2019

Eric-Arellano added a commit to Eric-Arellano/pants that referenced this pull request May 14, 2019

Setup MyPy config and enforce types for build-support scripts (pantsb…
…uild#7704)

### Problem
We would like to use type hints throughout our code base, per pantsbuild#6742.

However, we must configure it with sensible defaults to make it easier to use and to provide a consistent interface for developers.

We should also enforce as much type safety as we can via our `pre-commit` check.

### Solution
* Add new `mypy.ini` file with fairly loose defaults that are good for first time codebases. Eventually, we will want to do things like require all functions to be typed, but for now we loosen our expectations.
* Link to the config file in `pants.ini`, along with using the most modern MyPy release which gives us  the `strict_equality` check.
* Run `mypy.py` over the build-support scripts, which are fully typed.
   * Acts as a proof of concept for how this will be integrated into CI.

### Result
`./pants mypy` is now a bit more useful on our codebase, even though it will still fail because of pre-existing issues.

Further, the build-support folder will not have any typing regressions now.
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.