Skip to content

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

Merged
merged 6 commits into from
Feb 17, 2023
Merged

DOC: run typing checks #51352

merged 6 commits into from
Feb 17, 2023

Conversation

twoertwein
Copy link
Member

@twoertwein twoertwein requested a review from Dr-Irv February 12, 2023 22:34
Copy link
Contributor

@Dr-Irv Dr-Irv left a 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>`.
Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Member

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

Copy link
Contributor

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.

Copy link
Member

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)

Copy link
Member Author

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.
Copy link
Contributor

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."

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you!

Copy link
Contributor

@Dr-Irv Dr-Irv left a comment

Choose a reason for hiding this comment

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

Thanks @twoertwein

@Dr-Irv Dr-Irv merged commit 89bafd2 into pandas-dev:main Feb 17, 2023
@twoertwein twoertwein deleted the typing_doc branch August 9, 2023 15:08
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.

Instructions for checking typing of code cause pylint and autotyping to run
3 participants