Skip to content

Instructions for checking typing of code cause pylint and autotyping to run #51313

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

Closed
Dr-Irv opened this issue Feb 10, 2023 · 6 comments · Fixed by #51352
Closed

Instructions for checking typing of code cause pylint and autotyping to run #51313

Dr-Irv opened this issue Feb 10, 2023 · 6 comments · Fixed by #51352
Assignees
Labels
Code Style Code style, linting, code_checks Docs Typing type annotations, mypy/pyright type checking

Comments

@Dr-Irv
Copy link
Contributor

Dr-Irv commented Feb 10, 2023

At https://pandas.pydata.org/docs/dev/development/contributing_codebase.html#validating-type-hints we tell people to validate the types of their code by doing: pre-commit run --hook-stage manual --all-files . The problem here is that this causes autotyping and pylint to run.

pylint takes a long time, so we should tell people to skip it.
Worse is that autotyping causes lots of files to change, so you end up with a bigger set of changed files.

I think we should modify the .pre-commit-config.yaml to define a typing stage that only runs pyright and mypy.

Even if we do that, I get a lot of typing errors on the code in main from both type checkers. So that should be addressed as well.

@Dr-Irv Dr-Irv added Docs Code Style Code style, linting, code_checks Typing type annotations, mypy/pyright type checking labels Feb 10, 2023
@MarcoGorelli
Copy link
Member

Hey

Even if we do that, I get a lot of typing errors on the code in main from both type checkers. So that should be addressed as well.

CI is green - do you have the same version(s) of packages installed? I've sometimes had failures when I had, for example, a different numpy version

@twoertwein
Copy link
Member

I think we should modify the .pre-commit-config.yaml to define a typing stage that only runs pyright and mypy.

I think we can define only one manual stage (which started out to contain only mypy+pyright and then pylint+autotyping were moved there as well).

Could change the typing documentation to:

pre-commit run --hook-stage manual mypy
pre-commit run --hook-stage manual pyright
pre-commit run --hook-stage manual pyright_reportGeneralTypeIssues

@Dr-Irv
Copy link
Contributor Author

Dr-Irv commented Feb 12, 2023

Could change the typing documentation to:

pre-commit run --hook-stage manual mypy
pre-commit run --hook-stage manual pyright
pre-commit run --hook-stage manual pyright_reportGeneralTypeIssues

Yes, and you have to add that you need to stage the changed files before doing the typing check.

@Dr-Irv
Copy link
Contributor Author

Dr-Irv commented Feb 12, 2023

CI is green - do you have the same version(s) of packages installed? I've sometimes had failures when I had, for example, a different numpy version

So I had done mamba env update -f environment.yml, but because pandas-dev was activated, not all packages (including mypy) were updated. So by deactivating the environment, then doing the mamba update command, everything got properly updated.

Maybe we need to put something in the contributing docs about how to update the environment when you merge with upstream/main.

@jayam30
Copy link

jayam30 commented Feb 16, 2023

Hey I want to start my contribution with this, could I be assigned this

@Dr-Irv
Copy link
Contributor Author

Dr-Irv commented Feb 16, 2023

Hey I want to start my contribution with this, could I be assigned this

Thanks for the offer, but we have an active PR #51352 that has the required changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code Style Code style, linting, code_checks Docs Typing type annotations, mypy/pyright type checking
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants