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

ci(python): Run mypy as part of the lint workflow #6820

Merged
merged 1 commit into from
Feb 12, 2023

Conversation

stinodego
Copy link
Member

@stinodego stinodego commented Feb 12, 2023

Changes:

  • Add separate jobs in the lint workflow for running mypy on Python 3.7 and 3.11.
  • The part that of that workflow that lints the Rust code (rustfmt and clippy) is now a separate job

Advantages:

  • If there is a mypy error, we still get to see whether our tests pass.
  • It makes sense for mypy to be run as part of the lint workflow
  • With the Rust part separated out, the overall runtime of the lint job is now about 30 seconds shorter. (which doesn't matter much, as the Windows test workflow is the bottleneck)

Disadvantages:

  • Costs slightly more compute to run things this way, as we have to install dev dependencies twice. This is hopefully offset by the fact that people will require fewer iterations to pass all workflows, as test failures become apparent more quickly.

@github-actions github-actions bot added ci Related to the continuous integration setup python Related to Python Polars labels Feb 12, 2023
@ritchie46
Copy link
Member

Seems fine by me. 👍 I don't believe contention of workers is that high yet.

@stinodego
Copy link
Member Author

All right, then it's going in! Let me know if any issues arise.

@stinodego stinodego merged commit 52ffd22 into pola-rs:master Feb 12, 2023
@stinodego stinodego deleted the mypy-ci branch February 22, 2023 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci Related to the continuous integration setup python Related to Python Polars
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants