Skip to content

DOC: Contributor guideline should guide contributors for expected local mypy failures #34902

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
oguzhanogreden opened this issue Jun 20, 2020 · 9 comments · Fixed by #35066
Closed
Assignees
Labels
Code Style Code style, linting, code_checks good first issue Typing type annotations, mypy/pyright type checking

Comments

@oguzhanogreden
Copy link
Contributor

Location of the documentation

Location.

Documentation problem

See conversation here.

Suggested fix for documentation

This 'failing' of mypy should be noted so that inexperienced contributors don't have to Google around and experienced ones don't have to explain repeatedly.

I came across this in this context.

@oguzhanogreden oguzhanogreden added Docs Needs Triage Issue that has not been reviewed by a pandas team member labels Jun 20, 2020
@oguzhanogreden
Copy link
Contributor Author

SKIP=mypy may be the 'clean' suggestion, thanks @MarcoGorelli.

@MarcoGorelli MarcoGorelli added good first issue and removed Needs Triage Issue that has not been reviewed by a pandas team member labels Jun 21, 2020
@MarcoGorelli
Copy link
Member

Thanks @oguzhanogreden , might be good to add a line mentioning this to the "pre-commit" section of the contributing guide https://pandas.pydata.org/pandas-docs/dev/development/contributing.html#pre-commit

@erfannariman
Copy link
Member

take

@erfannariman
Copy link
Member

Need some input for this, based on the discussion in gitter (see link in message by OP), we should use

SKIP=mypy git commit -m "<some message which I know should be descriptive but usually isn't>"

But this does not work for me, I had to use

git commit --no-verify -m "<some message>"

Which one are we going for and should I add to the docs?

@WillAyd
Copy link
Member

WillAyd commented Jun 22, 2020

Why not just remove mypy from the pre-commit check? I think this has been an ongoing issue with the interaction of those tools @TomAugspurger I think brought up originally

@erfannariman
Copy link
Member

erfannariman commented Jun 22, 2020

If it (mypy check) is not needed, @WillAyd 's suggestion makes more sense to me.

@WillAyd
Copy link
Member

WillAyd commented Jun 22, 2020

Another option would be to only run mypy before a push instead of on every commit. This might be achievable with specifying the stage in the config

https://pre-commit.com/

@MarcoGorelli
Copy link
Member

MarcoGorelli commented Jun 27, 2020

Another option would be to only run mypy before a push instead of on every commit. This might be achievable with specifying the stage in the config

https://pre-commit.com/

Not sure that work would - see below

(pandas-dev) marco@marco-Predator-PH315-52:~/pandas-dev$ git fetch upstream master
From https://github.com/pandas-dev/pandas
 * branch                master     -> FETCH_HEAD
(pandas-dev) marco@marco-Predator-PH315-52:~/pandas-dev$ git reset --hard upstream/master
HEAD is now at 0159cba6e CLN: dont consolidate in indexing (#34679)
(pandas-dev) marco@marco-Predator-PH315-52:~/pandas-dev$ pre-commit run --all
black....................................................................Passed
flake8...................................................................Passed
flake8-pyx...............................................................Passed
flake8-pxd...............................................................Passed
isort....................................................................Failed
- hook id: isort
- files were modified by this hook

Fixing /home/marco/pandas-dev/web/pandas_web.py

mypy.....................................................................Failed
- hook id: mypy
- exit code: 1

pandas/core/dtypes/generic.py:10: error: unused 'type: ignore' comment
pandas/core/resample.py:970: error: unused 'type: ignore' comment
Found 2 errors in 2 files (checked 1043 source files)

(pandas-dev) marco@marco-Predator-PH315-52:~/pandas-dev$ mypy pandas
Success: no issues found in 1024 source files

IMHO it could be removed from pre-commit - if you're adding type annotations you'll need to run mypy pandas anyway, and if you're not then its false positives are a distraction (especially for new contributors)

@TomAugspurger
Copy link
Contributor

I'm fine with removing mypy from pre-commit.

@MarcoGorelli MarcoGorelli added Code Style Code style, linting, code_checks Typing type annotations, mypy/pyright type checking and removed Docs labels Jul 4, 2020
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 good first issue Typing type annotations, mypy/pyright type checking
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants