Skip to content
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

Type annotations for help, list and uninstall in commands #8016

Merged
merged 4 commits into from
May 17, 2020

Conversation

deveshks
Copy link
Contributor

@deveshks deveshks commented Apr 10, 2020

Towards #4748

Added type annotations to pip._internal.commands.help, pip._internal.commands.list and pip._internal.commands.uninstall

@deveshks deveshks force-pushed the add-mypy-annotations-commands branch 2 times, most recently from bba104a to d28d117 Compare April 10, 2020 21:54
@deveshks deveshks changed the title Type annotations to _internal.commands.help, list Type annotations for help, list and uninstall in commands Apr 10, 2020
@deveshks deveshks force-pushed the add-mypy-annotations-commands branch from d28d117 to 7901ba6 Compare April 10, 2020 22:02
@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 😊

@deveshks deveshks force-pushed the add-mypy-annotations-commands branch from 7901ba6 to f138839 Compare April 13, 2020 19:32
src/pip/_internal/commands/list.py Outdated Show resolved Hide resolved
src/pip/_internal/commands/uninstall.py Outdated Show resolved Hide resolved
src/pip/_internal/commands/list.py Outdated Show resolved Hide resolved
@deveshks deveshks force-pushed the add-mypy-annotations-commands branch from f138839 to fe134b6 Compare April 14, 2020 04:57
@deveshks deveshks closed this Apr 14, 2020
@deveshks deveshks reopened this Apr 14, 2020
@deveshks deveshks force-pushed the add-mypy-annotations-commands branch from fe134b6 to c0a1285 Compare April 14, 2020 05:24
@deveshks
Copy link
Contributor Author

deveshks commented Apr 14, 2020

For some reason, linting is failing with https://github.com/pypa/pip/pull/8016/checks?check_run_id=584694795#step:8:29 and https://travis-ci.org/github/pypa/pip/jobs/674711045#L314 as src/pip/_internal/commands/list.py:203: error: Function is missing a type annotation , but I don't have a function definition on line 203

https://github.com/deveshks/pip/blob/fe134b6a1295320916cc1aef1e5703ef3da35940/src/pip/_internal/commands/list.py#L203

Linting works perfectly fine on my development Mac with Python3.8 with OSX 10.15.4 (19E266)

$ git status
On branch add-mypy-annotations-commands
Your branch is up to date with 'origin/add-mypy-annotations-commands'.

nothing to commit, working tree clean
$ git branch
* add-mypy-annotations-commands
  master
$ git log --format="%H" -n 1
c0a128570b10932ad36d813fdfadfc7b5d37eca0

$ tox -e lint
lint installed: appdirs==1.4.3,cfgv==3.1.0,distlib==0.3.0,filelock==3.0.12,identify==1.4.14,nodeenv==1.3.5,pip==20.0.2,pre-commit==2.2.0,PyYAML==5.3.1,setuptools==46.1.3,six==1.14.0,toml==0.10.0,virtualenv==20.0.17,wheel==0.34.2
lint run-test-pre: PYTHONHASHSEED='2304054552'
lint run-test: commands[0] | pre-commit run --all-files --show-diff-on-failure
Check builtin type constructor use.......................................Passed
Check for added large files..............................................Passed
Check for case conflicts.................................................Passed
Check Toml...............................................................Passed
Check Yaml...............................................................Passed
Debug Statements (Python)................................................Passed
Fix End of Files.........................................................Passed
Flake8...................................................................Passed
Forbid new submodules....................................................Passed
Trim Trailing Whitespace.................................................Passed
isort....................................................................Passed
mypy.....................................................................Passed
mypy, for Py2............................................................Passed
use logger.warning(......................................................Passed
check for eval().........................................................Passed
rst ``code`` is two backticks............................................Passed
check-manifest...........................................................Passed
____________________________________________________ summary ____________________________________________________
  lint: commands succeeded
  congratulations :)

Edit: The linting checks are still failing, even after I merged master into the PR branch, and the Linting push checks in my fork just after I merged master https://github.com/deveshks/pip/runs/585435303 are also failing.

Edit II: So finally I am able to get all checks passing by pulling in master, and adding a type annotation to an inner function which was added in a later commit before this PR was created, at the same line where I was getting the error: Function is missing a type annotation

For some reason, when github was running it's CI checks, it was taking the parts of list.py which wasn't modified by me from the latest master branch and overlaying it over the file, instead of taking the entire file from the PR branch, which was causing the issue. Not sure if this is an intended behaviour or a bug.

What are your thoughts on this @pradyunsg @uranusjr @pfmoore ?

@deveshks deveshks requested a review from sbidoul April 14, 2020 06:26
src/pip/_internal/commands/list.py Outdated Show resolved Hide resolved
src/pip/_internal/commands/list.py Outdated Show resolved Hide resolved
src/pip/_internal/commands/list.py Outdated Show resolved Hide resolved
@deveshks deveshks requested a review from sbidoul April 14, 2020 07:56
@deveshks deveshks force-pushed the add-mypy-annotations-commands branch from adf3c9f to 0470a82 Compare April 14, 2020 10:37
@deveshks deveshks requested a review from pfmoore April 14, 2020 11:29
@deveshks deveshks force-pushed the add-mypy-annotations-commands branch 3 times, most recently from 5314e57 to 52f30a1 Compare April 19, 2020 21:58
@deveshks
Copy link
Contributor Author

deveshks commented Apr 27, 2020

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

@McSinyx
Copy link
Contributor

McSinyx commented Apr 27, 2020

Changes looking good to me personally, however the maintainers are rushing for (patches for) 20.1 release I think.

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.

This LGTM now, thank you!

@deveshks
Copy link
Contributor Author

This LGTM now, thank you!

Thanks for the approval @sbidoul 😊

@pradyunsg pradyunsg removed the request for review from pfmoore May 17, 2020 17:43
@pradyunsg pradyunsg merged commit 301f65f into pypa:master May 17, 2020
@pradyunsg pradyunsg added the type: maintenance Related to Development and Maintenance Processes label May 17, 2020
@deveshks deveshks deleted the add-mypy-annotations-commands branch May 17, 2020 17:50
@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.

6 participants