Skip to content

Added type annotations to pip._internal.commands.check #7977

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 9, 2020
Merged

Added type annotations to pip._internal.commands.check #7977

merged 1 commit into from
Apr 9, 2020

Conversation

deveshks
Copy link
Contributor

@deveshks deveshks commented Apr 4, 2020 •
edited
Loading

Towards #4748

Added type annotations to pip._internal.commands.check

@deveshks deveshks force-pushed the add-mypy-annotations-commands branch from 77f4473 to ed3561e Compare April 4, 2020 09:36
@deveshks deveshks force-pushed the add-mypy-annotations-commands branch from ed3561e to 21b5357 Compare April 4, 2020 13:33
@deveshks deveshks requested a review from sbidoul April 4, 2020 14:08
@deveshks deveshks force-pushed the add-mypy-annotations-commands branch from 21b5357 to e50efcd Compare April 4, 2020 15:38
pradyunsg
pradyunsg requested changes Apr 4, 2020
View reviewed changes
Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

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

LGTM, minus one annotation that needs to be changed.

@deveshks deveshks requested a review from pradyunsg April 4, 2020 18:54
pradyunsg
pradyunsg requested changes Apr 4, 2020
View reviewed changes
Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

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

This still needs changes.

@deveshks
Copy link
Contributor Author

deveshks commented Apr 4, 2020

This still needs changes.

Could you specify where in the code do I need to change :) I will update accordingly

@deveshks deveshks requested a review from pradyunsg April 4, 2020 19:29
@pradyunsg
Copy link
Member

@deveshks We'd want to fix the annotation in upstream typeshed, and then come back to this PR once there's a new mypy release w/ an updated typeshed.

Until then, I don't think we should add a misleading type annotation to our codebase. :)

@deveshks deveshks force-pushed the add-mypy-annotations-commands branch from e50efcd to 3c4452f Compare April 4, 2020 20:17
@deveshks
Copy link
Contributor Author

deveshks commented Apr 4, 2020 •
edited
Loading

Hi @pradyunsg

As per your suggestion, I have reverted the type annotations added to pip._internal.commands.__init__ due to the incorrect return type of difflib.get_closest_matches , as reported in python/typeshed#3906 and python/typeshed#2067

@deveshks deveshks changed the title Add mypy annotations to init and check in commands Added type annotations to pip._internal.commands.check Apr 4, 2020
@deveshks deveshks force-pushed the add-mypy-annotations-commands branch from 3c4452f to c2fdf4a Compare April 4, 2020 20:26
@deveshks
Copy link
Contributor Author

deveshks commented Apr 6, 2020

Hi @pradyunsg

If there are no more concerns with this PR, could I get it approved/merged 😊

I will keep future PR related to mypy annotations at one file per PR, as per your comment at #4748 (comment) . It does make it easier to get reviewed.

@pradyunsg pradyunsg merged commit c8e4afa into pypa:master Apr 9, 2020
@deveshks deveshks deleted the add-mypy-annotations-commands branch April 9, 2020 11:47
This was referenced Apr 12, 2020
Merged
Merged
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label May 21, 2020
@lock lock bot locked as resolved and limited conversation to collaborators May 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@McSinyx McSinyx McSinyx left review comments

@pradyunsg pradyunsg pradyunsg approved these changes

@sbidoul sbidoul Awaiting requested review from sbidoul

Assignees
No one assigned
Labels
auto-locked Outdated issues that have been locked by automation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@deveshks @pradyunsg @sbidoul @McSinyx