Skip to content

Complete type annotations in pip/_internal/index #10111

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 18 commits into from
Jul 12, 2021

Conversation

DiddiLeija
Copy link
Member

Convert type hint commentaries into annotations, this time for pip/_internal/index.

Convert type hint commentaries into annotations for `pip/_internal/index`
This is the news entry for my pull request.
@DiddiLeija
Copy link
Member Author

DiddiLeija commented Jun 29, 2021

The ci / pre-commit check failed with flake8:

flake8...................................................................Failed
- hook id: flake8
- exit code: 1

src/pip/_internal/index/collector.py:269:30: F821 undefined name 'HTMLPage'
src/pip/_internal/index/collector.py:282:19: F821 undefined name 'HTMLPage'
src/pip/_internal/index/collector.py:283:16: F821 undefined name 'HTMLPage'
src/pip/_internal/index/collector.py:304:23: F821 undefined name 'HTMLPage'
src/pip/_internal/index/collector.py:372:89: E501 line too long (91 > 88 characters)
src/pip/_internal/index/collector.py:451:89: E501 line too long (108 > 88 characters)
src/pip/_internal/index/collector.py:451:95: F821 undefined name 'LinkCollector'

Maybe the original annotations were wrong. How can I fix them?

@pradyunsg
Copy link
Member

pradyunsg commented Jun 29, 2021

What's happening is that HTMLPage is defined after the function you're annotating.

The comments didn't hit any issue, since they aren't executed by the Python interpreter, so only mypy is doing a lookup. Changing the failing HTMLPage annotations to "HTMLPage" would fix this issue. Same for LinkCollector.

@DiddiLeija
Copy link
Member Author

What's happening is that HTMLPage is defined after the function you're annotating.

The comments didn't hit any issue, since they aren't executed by the Python interpreter, so only mypy is doing a lookup. Changing the failing HTMLPage annotations to "HTML Page" would fix this issue. Same for LinkCollector

Ok. I will take that tomorrow (it's late here). Thanks @pradyunsg!

Copy link
Member

@uranusjr uranusjr left a comment

Choose a reason for hiding this comment

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

I think the HTMLPage annotations need to use strings (forward reference issues). There are a few other linter errors as well.

https://mypy.readthedocs.io/en/stable/common_issues.html#issues-with-code-at-runtime

@DiddiLeija
Copy link
Member Author

DiddiLeija commented Jul 2, 2021

I think the HTMLPage annotations need to use strings (forward reference issues). There are a few other linter errors as well.

https://mypy.readthedocs.io/en/stable/common_issues.html#issues-with-code-at-runtime

Ok, got it. Unfortunately, I think I will be a quite busy for this month (things are getting harder on the real world!). But maybe the next week I can take a look for that (It's Friday here). Thanks @uranusjr!

@harupy
Copy link
Contributor

harupy commented Jul 4, 2021

@DiddiLeija @uranusjr @pradyunsg Can I work on type-comments to type-annotations translation as well?

I have worked on something similar in https://github.com/optuna/optuna and would like to help.

@uranusjr
Copy link
Member

uranusjr commented Jul 4, 2021

Feel free, as long as you don’t step on each other’s toes 🙂

@DiddiLeija
Copy link
Member Author

DiddiLeija commented Jul 4, 2021

Hey @harupy. Thanks!

Maybe we can coordinate, to be more accurate on our work. I will work on this folders:

pip/_internal/models
pip/_internal/utils
pip/_internal/vcs

And maybe you can take the rest, or someone else could take them (they are a lot of files, so any kind of help is useful...).

@harupy
Copy link
Contributor

harupy commented Jul 4, 2021

@DiddiLeija Got it!

@harupy
Copy link
Contributor

harupy commented Jul 4, 2021

I'm using this package to automatically translate type comments into type annotations:
https://github.com/ilevkivskyi/com2ann

@DiddiLeija
Copy link
Member Author

I finally fixed the collector.py issues. Thanks @uranusjr and @pradyunsg for your guide.

@DiddiLeija
Copy link
Member Author

Side note: I wanted to say Fix the pip/_internal/index/package_finder.py annotations, but it was too large and I had a mistake.

I annotated an argument wrong.
Seems like "tag" is not defined, so I just annotated as a string.
@DiddiLeija
Copy link
Member Author

Hum... Seems like there is a wrong annotation. Look at this error message:

mypy.....................................................................Failed
- hook id: mypy
- exit code: 1

src/pip/_internal/index/package_finder.py:412: error: Name 'tag' is not defined
            supported_tags: List["tag"],
                                 ^
Found 1 error in 1 file (checked 150 source files)

Even when the annotation is a string, it fails! Maybe the annotation is wrong. Can somebody tell me what's going on here?

@DiddiLeija DiddiLeija requested a review from uranusjr July 5, 2021 15:34
Maybe "tag" was originally "Tag". I fixed that simple mistake.
@DiddiLeija
Copy link
Member Author

Oh, forget it. It was Tag, not tag 🤦‍♂️.

These changes were suggested by the flake8 test.
flake8 told me they were too long.
@DiddiLeija
Copy link
Member Author

DiddiLeija commented Jul 5, 2021

I've done the pip/_internal/index annotations (Seems like sources.py was already converted by @uranusjr on commit a912c55). Now it is ready to be reviewed by someone. What do you think about it?

@uranusjr uranusjr merged commit ce86dc8 into pypa:main Jul 12, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants