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

Complete type hints of tests/unit/ directory #10435

Merged
merged 1 commit into from
Sep 26, 2021
Merged

Complete type hints of tests/unit/ directory #10435

merged 1 commit into from
Sep 26, 2021

Conversation

jdufresne
Copy link
Contributor

Adding these types exposed some tests that were passing invalid values
to functions.

These types also allowed refining some existing APIs.

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 did not read all the type hint additions in the test suite, but assume they are fine since Mypy is passing. The type hint changes in pip proper are all correct.

@github-actions github-actions bot added the needs rebase or merge PR has conflicts with current master label Sep 5, 2021
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Sep 5, 2021
@jdufresne
Copy link
Contributor Author

I did not read all the type hint additions in the test suite, but assume they are fine since Mypy is passing.

If you'd like me to break this down to multiple PRs to assist review, please let me know. I can either chunk it to fewer file changes at a time or if you notice standalone changes I can tease those out. If that is desirable, please let me know, it is no problem.

@github-actions github-actions bot added the needs rebase or merge PR has conflicts with current master label Sep 12, 2021
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Sep 12, 2021
@github-actions github-actions bot added the needs rebase or merge PR has conflicts with current master label Sep 20, 2021
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Sep 21, 2021
@github-actions github-actions bot added the needs rebase or merge PR has conflicts with current master label Sep 21, 2021
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Sep 23, 2021
"""
Call command.main(), and return the command's stderr.
"""

def raise_broken_stdout():
def raise_broken_stdout() -> int:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def raise_broken_stdout() -> int:
def raise_broken_stdout() -> NoReturn:

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.

I'm at 44/64, and this looks pretty good! :)

Comment on lines 69 to 77
entry = wc.get_cache_entry(persi_link, "persi", supported_tags)
assert entry is not None
assert entry.persistent
entry = wc.get_cache_entry(ephem_link, "ephem", supported_tags)
assert entry is not None
assert not entry.persistent
assert wc.get_cache_entry(other_link, "other", supported_tags) is None
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
entry = wc.get_cache_entry(persi_link, "persi", supported_tags)
assert entry is not None
assert entry.persistent
entry = wc.get_cache_entry(ephem_link, "ephem", supported_tags)
assert entry is not None
assert not entry.persistent
assert wc.get_cache_entry(other_link, "other", supported_tags) is None
entry = wc.get_cache_entry(persi_link, "persi", supported_tags)
assert entry is not None
assert entry.persistent
entry = wc.get_cache_entry(ephem_link, "ephem", supported_tags)
assert entry is not None
assert not entry.persistent
assert wc.get_cache_entry(other_link, "other", supported_tags) is None

I like whitespace.

Comment on lines 681 to 685
metadata = email.message.Message()
metadata["name"] = "simplewheel"
metadata["version"] = "1.0"
req._metadata = metadata
req.assert_source_matches_version()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
metadata = email.message.Message()
metadata["name"] = "simplewheel"
metadata["version"] = "1.0"
req._metadata = metadata
req.assert_source_matches_version()
metadata = email.message.Message()
metadata["name"] = "simplewheel"
metadata["version"] = "1.0"
req._metadata = metadata
req.assert_source_matches_version()

As I was saying, I like my whitespace.

@jdufresne
Copy link
Contributor Author

Thanks for the review. I applied all suggestions in the latest revision.

@pradyunsg
Copy link
Member

pradyunsg commented Sep 24, 2021

FWIW, for large PRs like this, I'd encourage you to make commit --fixup for changes and doing a single rebase --autosquash once approved, prior to merging. It makes reviewing the actual changes easier. :)

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.

Alrighty. GitHub says I've looked at each file and marked it as viewed. :)

@uranusjr uranusjr merged commit b392833 into pypa:main Sep 26, 2021
@jdufresne jdufresne deleted the typing-unit branch September 30, 2021 11:07
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 16, 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.

5 participants