-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
DOC: run typing checks #51352
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
DOC: run typing checks #51352
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In line 269, we have this:
"after making any change you can ensure your type hints are correct by running"
What I've found is that you have to stage your changes via git add
in order for the commands you provided to work. So I think the docs should be modified to say that as well.
in your activated python environment. A recent version of ``numpy`` (>=1.22.0) is required for type validation. | ||
.. warning:: | ||
|
||
* It is essential that the versions of ``mypy`` and ``numpy`` are the same as used on main. Please see :ref:`how to setup the python environment <contributing.mamba>`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should mention numpy
here as we don't specify a version of numpy
in environment.yml
or requirements-dev.txt
.
Secondly, can we use this PR to address this comment: #51313 (comment) ?
The contributing.mamba
section doesn't indicate how you should update your mamba
environment when versions change. This has bitten me multiple times, so it would be good to put something in the docs there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't use mamba/conda so I don't feel comfortable commenting on how to update the environment.
My simple/stupid approach: If mypy/pyright fails, I just check which numpy/mypy version main installs for the most recently merged PR and then pip install the same version :) Just matching the numpy/mypy version has so far always worked for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't use mamba/conda so I don't feel comfortable commenting on how to update the environment.
OK, maybe I will do a PR about that.
My simple/stupid approach: If mypy/pyright fails, I just check which numpy/mypy version main installs for the most recently merged PR and then pip install the same version :) Just matching the numpy/mypy version has so far always worked for me.
Yes, but you're smarter about these things than our average contributor! I've found these kinds of issues a PITA to figure out what is wrong with my environment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think we should delete numpy
from this part of the doc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Dr-Irv - what do you suggest this be replaced with if you don't want to mention numpy?
I also use the approach of checking which version was running in CI, and checking my environment matches that. I think that'd be fine to document
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Dr-Irv - what do you suggest this be replaced with if you don't want to mention numpy?
My concern here is that the sentence is saying "what is used in main", but there is no specific version on main
. The version is determined by what is used in CI. So I'm not exactly sure what to say.
I also use the approach of checking which version was running in CI, and checking my environment matches that. I think that'd be fine to document
Think of it from a first-time contributor point of view. How are they supposed to figure out what was running in CI when they haven't pushed anything yet? Documenting how to look in the CI (or somewhere else?) would be fine to add here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, rephrasing "what is used in main" to something to the effect of "what was used in the latest CI build", and mentioning which CI job they should be looking at (i.e. the one which runs mypy)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to describe how to navigate to the section of a pandas CI report where the versions are listed (not sure whether that is clear enough - probably not).
in your activated python environment. A recent version of ``numpy`` (>=1.22.0) is required for type validation. | ||
.. warning:: | ||
|
||
* Please be aware that the above commands will use the current python environment. If your python packages are older/newer than those installed by the pandas CI, the above commands might fail. This is often the case when the ``mypy`` or ``numpy`` versions do not match. Please see :ref:`how to setup the python environment <contributing.mamba>` or click on a recent CI report, then click on "Set up Conda" and "Environment info" to see which versions the pandas CI installs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might want to say "Look at https://github.com/pandas-dev/pandas/actions/workflows/code-checks.yml for a successful code check, then click on 'Docstring validation, typing, and other manual pre-commit hooks' , then click on Set up Conda" and "Environment info" to see which versions the pandas CI installs."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @twoertwein
doc/source/whatsnew/vX.X.X.rst
file if fixing a bug or adding a new feature.