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

Add mypy to continuous integration suite #2432

Merged
merged 7 commits into from Jan 5, 2024

Conversation

namurphy
Copy link
Member

@namurphy namurphy commented Dec 29, 2023

Description

This PR adds mypy to our continuous integration suite.

Motivation and context

mypy is the de facto static type checker in the pythoniverse. It analyzes source code directly and makes sure that type hint annotations are used consistently throughout the package.

Adopting mypy in our CI will help us:

  • Find bugs and common programming errors.
  • Ensure our type hint annotations are accurate and complete.
  • Better understand what our functions are doing, especially in complicated situations.

Additional considerations or tradeoffs are:

  • Code with type hint annotations is more verbose.
  • We would need a good way to introduce type hint annotations to new contributors, such as in the contributor guide and in the comment that gets posted to every PR. We can point to mypy's typing cheatsheet, and discuss some considerations that are specific to PlasmaPy.
  • I've found error messages from mypy to...take some getting used to. Additionally, mypy may point to an error on a particular line, when it's a different line that actually needs fixing.
  • mypy is unable to type check objects that are created dynamically rather than being defined statically in the source code (see also Add a type stub file for dynamically generated unit objects in astropy.units astropy/astropy#15808).
  • Our ability to type check things will be limited when upstream packages are not typed, including astropy.units. We can also add type stub files (like in Add type stub file for @wrapt.decorator #2442) for important functionality, or # type: ignore[error-code] comments for less important situations. These would need only include the functionality that we actually use.

Related issues

Closes #268, which was raised almost six years ago!

In #2424, we added a mypy configuration file that ignores existing errors on a per-file and per-error basis. In #2431, we added a tox environment for running mypy. This PR adds that tox environment to our suite of continuous integration checks.

We also changed @validate_quantities to work with u.Quantity[u.m] style comments in #2346, following by actual changes to the annotations in #2356 and #2421.

@github-actions github-actions bot added CI Related to continuous integration GitHub Actions A continuous integration platform for automating tests and other tasks (see .github/ directory) labels Dec 29, 2023
Copy link

codecov bot commented Dec 29, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (41b00c1) 96.92% compared to head (e14f3e4) 96.92%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2432   +/-   ##
=======================================
  Coverage   96.92%   96.92%           
=======================================
  Files         104      104           
  Lines        9153     9153           
=======================================
  Hits         8872     8872           
  Misses        281      281           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@namurphy namurphy marked this pull request as ready for review December 29, 2023 23:40
@namurphy namurphy requested a review from a team as a code owner December 29, 2023 23:40
@namurphy namurphy requested review from ejohnson-96 and removed request for a team December 29, 2023 23:40
@@ -158,4 +158,4 @@ commands = python -c 'import plasmapy'
deps =
mypy >= 1.8.0
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if there's an alternative to having to update this manually... dependabot, maybe?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔

@@ -158,4 +158,4 @@ commands = python -c 'import plasmapy'
deps =
mypy >= 1.8.0
commands =
mypy plasmapy --install-types --non-interactive
mypy plasmapy --install-types --non-interactive --show-error-context --show-error-code-links --pretty
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll probably want to play with these flags to figure out which error output works best.

@namurphy
Copy link
Member Author

It's still really surprising to see mypy passing in our CI suite! I mean...we've got it set to ignore ∼700 errors, but still! 😺

Copy link
Contributor

@ejohnson-96 ejohnson-96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 All looks good

Copy link
Member

@StanczakDominik StanczakDominik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks quite sensible, let's try it out :) Please allow me to do something I haven't done in a while...

@StanczakDominik StanczakDominik merged commit 90fe228 into PlasmaPy:main Jan 5, 2024
19 checks passed
@namurphy
Copy link
Member Author

namurphy commented Jan 5, 2024

Thank you for the reviews! I'm curious about what @rocco8773 thinks about this too. I'm really happy to have this added to our CI suite since it's been something I've been hoping to do for several years.

Enabling mypy will level up the quality of our code and documentation, but my main worry is that it'll be an extra barrier to entry for new contributors. So, we should keep thinking of our mypy configuration as experimental. I currently have it set so that new modules are expected to follow mypy's strict settings, but we may want to loosen those settings later on for some modules.

@StanczakDominik — It's super awesome to hear from you and I hope you've been doing really well!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Related to continuous integration GitHub Actions A continuous integration platform for automating tests and other tasks (see .github/ directory) static type checking testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use mypy for static type checking in continuous integration
3 participants