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 type checker to Github Actions #1584

Closed
terriko opened this issue Feb 22, 2022 · 11 comments
Closed

Add type checker to Github Actions #1584

terriko opened this issue Feb 22, 2022 · 11 comments
Labels
enhancement New feature or request
Milestone

Comments

@terriko
Copy link
Contributor

terriko commented Feb 22, 2022

Now that we're starting to get type hints in more files, I'd like to have CI give us feedback on type hints.

This can be allowed to fail on most files right now. I don't love the way github actions handles that (last I checked, it made "allow fail" items look like they've passed even if they haven't so people may not look at them) but even if I'm not sure new contributors will know to look, it will be useful for more experienced contributors and folk reviewing pull requests to have the report generated for us.

As we get files into a good state, we could add them to the list of files where type checker fails are no longer allowed, and eventually the whole code base should be required to pass the type checker tests.

mypy was the top of my list at one point but python type checkers generally follow the same PEPs so if something else works better we can go there. I use Visual Studio Code while reviewing markdown files so a recommendation for tooling that works with that particular IDE might be nice for our documentation too.

Some talks that might be useful for someone interested in this issue:

@terriko terriko added the enhancement New feature or request label Feb 22, 2022
@terriko
Copy link
Contributor Author

terriko commented Feb 23, 2022

  • mypy: oldest, bit slower to update, well-supported by tools/CI
  • pyright : good support for vscode, faster moving, already has 3.11 support, provided by Microsoft
  • there's also one from google (pytype?)

@rhythmrx9
Copy link
Contributor

I would suggest pyright, it has been working fine for me, switched to it from mypy.
mypy sometimes gives errors that are not very helpful.

@XDRAGON2002
Copy link
Contributor

Top two contenders would be mypy and pyright, (pytype and pyre are too lenient for type checking sometimes).
Both are extremely good for usage as well and very similar, key differences being,

Mypy works best if the codebase is already typed out or to get the codebase aggressively typed out. Does not do any inference, completely typing based. Sometimes throws unhelpful errors. The existing standard for python type checks.

Pyright on the other hand allows for friendly gradual typing of the codebase and also tries to infer the types itself via the code flow. Is also way faster than mypy, throws helpful errors, the fact that its treated as a first class citizen in vscode is also a plus.

I guess its a matter of when we plan to integrate type checking enforcement in ci, parallelly or after the existing codebase is typed.

@terriko
Copy link
Contributor Author

terriko commented Mar 2, 2022

I'm intending to integrate something in CI sooner rather than later. As in, if someone makes a workable pull request that doesn't break CI, I'd be willing to integrate it this week. I'd like any file that passes checks to have enforcement turned on, the rest can be added to the enforced list as they become ready. So yes, parallel integration with CI, enforcement on a per-file basis until the codebase is fully updated.

Sounds like there's a slight preference for pyright, so I'd recommend anyone working on this issue start there.

Note: I'd be willing to consider this as a potential short (175hr) gsoc project, but I'm a bit concerned that we might be able to finish it before applications closed, which is why it hasn't been added to the gsoc list directly.

@XDRAGON2002
Copy link
Contributor

I'll look into getting this implemented.

@XDRAGON2002
Copy link
Contributor

XDRAGON2002 commented Mar 12, 2022

Should we have this only in CI, or also in the pre-commit-hook?

I suppose we could enforce type checks locally once we have most of the code base typed out, as enforcing them right now would lead to a lot of fails while making commits. Or maybe exclude files that are not typed yet?

@rhythmrx9
Copy link
Contributor

I think type checking the files that already have type hints, in pre-commit would be very useful. These can be the files that may not fail type checking in CI.

@XDRAGON2002
Copy link
Contributor

I would suggest to take a look at the PR and the generated logs for pyright, to identify what all should we exclude from checking (imports, stubs, files/directories etc) and in general the pyright config file.

For now I suppose we should use pyrightconfig.json file for the config temporarily this is because I was also looking into #1595 to get it implemented so once we have moved to pyproject.toml we could get rid of many of these singular config files which could easily be moved to the .toml file.

@terriko
Copy link
Contributor Author

terriko commented Oct 25, 2022

Update: I switched from using pyright to mypy due to npm errors that were incredibly poorly timed, but thanks to the folk working on hacktoberfest we're getting close to being able to enable mypy across the repo. I don't know that I'm going to push for it on 3.2 (depends on how many more bugs I file and get fixes for this week) but I'm going to flag this with the future milestone so it keeps showing up on my dashboard.

@terriko terriko added this to the future milestone Oct 25, 2022
@XDRAGON2002
Copy link
Contributor

Yay! This has been an ongoing effort for about almost a year at this point, or even more than that if I remember correctly.

@terriko
Copy link
Contributor Author

terriko commented Jan 5, 2023

Closed via #2488

@terriko terriko closed this as completed Jan 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants