Skip to content

Type annotations for hash, show and wheel in commands #8018

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 3 commits into from
May 17, 2020

Conversation

deveshks
Copy link
Contributor

Towards #4748

Added type annotations to pip._internal.commands.hash, pip._internal.commands.show and pip._internal.commands.wheel

@deveshks
Copy link
Contributor Author

Hi @McSinyx , @pradyunsg , @sbidoul

This is my next attempt on adding mypy annotations on some other files in pip._internal.commands submodule, and since you guys helped me to get the last PR related to this effort (#7977) in shape, I would appreciate if you guys could look at this and provide your thoughts 😊

@McSinyx
Copy link
Contributor

McSinyx commented Apr 13, 2020

Hi, I don't think I'm in the position for my words to be seen as reviews, but more like personal thoughts:

  1. I'd much prefer fewer newlines, e.g. right after type comments. This is to reflect PEP 8:

    Use blank lines in functions, sparingly, to indicate logical sections.

  2. It is possible to type hint args and kwargs as, for example (*Any, **Any) -> None. This is more consistent with Python 3.5+ annotation of foo(*args: Any, **kwargs: Any) -> None, just in case we drop Python 2 support sometime in the future.

@deveshks
Copy link
Contributor Author

Thanks @McSinyx , I have removed the newlines after type comments.
I myself am not sure of using the (*Any, **Any) -> None notation, so I have left it as is. I will wait for other reviewers to comment on that switch

Copy link
Contributor

@gutsytechster gutsytechster left a comment

Choose a reason for hiding this comment

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

With respect to comment here #8016 (comment), I think you can make a few changes described below. :)

@deveshks deveshks force-pushed the add-mypy-annotations-hash-show-wheel branch 2 times, most recently from 001d8bc to 3b35b41 Compare April 19, 2020 21:54
@deveshks
Copy link
Contributor Author

deveshks commented May 1, 2020

I have applied to suggested changes to the PR. If they look good, could I please get this PR approved?

Copy link
Member

@sbidoul sbidoul left a comment

Choose a reason for hiding this comment

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

A small nit, otherwise LGTM.

@deveshks
Copy link
Contributor Author

A small nit, otherwise LGTM.

Thanks for the approval @sbidoul 😊

@pradyunsg pradyunsg added the type: maintenance Related to Development and Maintenance Processes label May 17, 2020
@pradyunsg pradyunsg merged commit 04f63ca into pypa:master May 17, 2020
@deveshks deveshks deleted the add-mypy-annotations-hash-show-wheel branch May 17, 2020 17:51
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: maintenance Related to Development and Maintenance Processes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants