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

type sparse.py, enable mypy in CI #773

Merged
merged 26 commits into from
Nov 1, 2024
Merged

Conversation

frrad
Copy link
Contributor

@frrad frrad commented Sep 2, 2024

This is on top of #772 - the diff will be a little cleaner once that is merged.

This PR:

  1. adds type hints for sparse.py. I have made no functional changes, only types.
  2. adds a pyproject.toml which blacklists all files which don't currently pass mypy (this is most of them)
  3. changes .github/workflows/pythonpackage.yml to run mypy
  4. removes the (I believe) unused .travis.yml config

@frrad frrad changed the title [WIP] typing for sparse.py type sparse.py Sep 2, 2024
@pchtsp
Copy link
Collaborator

pchtsp commented Oct 22, 2024

hello! thanks. Why the pyproject.toml? and why type hint sparse? also, could you check #755 as this was the current work for the type hinting.

@frrad frrad changed the title type sparse.py type sparse.py, enable mypy in CI Oct 22, 2024
@frrad
Copy link
Contributor Author

frrad commented Oct 22, 2024

Hello!

I have added pyproject.toml in order to configure mypy. The current configuration is such that mypy . will pass. In particular, the addition I made to your .gihub/... configuration file should successfully run mypy on future PRs.

By introducing this check, we ensure that future files are mypy'd from the start, and give ourselves a nice way to ratchet up the type coverage - you can do smaller PRs which remove one or two files from pyproject.toml and add types to them. I chose sparse.py because it was an easy starting point.

I did notice #755

  1. since sparse.py is so isolated you should just be able to "take theirs" when you merge this branch into it and have no conflicts with type annotations in other files.
  2. I think that PR is a compelling piece of evidence in favor of the incremental approach - instead of having a years-long back and forth over one monolithic change you can just take small improvements as they come with less back and forth.

Copy link
Collaborator

@pchtsp pchtsp left a comment

Choose a reason for hiding this comment

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

Thanks, I've left a couple more comments and we should merge afterwards.

Comment on lines 1 to 2
black==23.3.0
mypy==1.4.1
Copy link
Collaborator

Choose a reason for hiding this comment

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

please leave the versions open. That is:

black
mypy

@@ -0,0 +1,70 @@
[tool.mypy]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you just add a directory instead of each file? something like seems to work according to https://stackoverflow.com/a/78480102/6508131:

exclude = [
"examples/",
"doc",
"pulp/apis",
"pulp/solverdir",
]

@pchtsp pchtsp merged commit c06fe08 into coin-or:master Nov 1, 2024
16 checks passed
@frrad
Copy link
Contributor Author

frrad commented Nov 1, 2024

Thanks for merging!

FYI the disadvantage of blacklisting folders is that new files added in these folders will be blacklisted be default.

Another option if you don't like the long list is to just add ignore comments to the tops of all the existing files. I'm happy to implement either of these as a separate PR if you're interested.

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 this pull request may close these issues.

2 participants