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

pkg_resources: fix incorrect implicit return types #4391

Merged
merged 1 commit into from
Jun 17, 2024

Conversation

Avasam
Copy link
Contributor

@Avasam Avasam commented May 23, 2024

Summary of changes

Checked all public methods (def ([^_]|__)) in pkg_resources for their implied return type using pyright/pylance. Updated annotations for those that seemed incorrect or returned a partial Any even after #4390 .

Step 3.3 of #2345 (comment)

Once this #4355 and #4390 are merged, pkg_setuptools will be on par with Typeshed

Pull Request Checklist

  • Changes have tests (type-checking and existing basic tests for syntax errors)
  • News fragment added in newsfragments/. (no user facing changes yet until pkg_resources is considered typed)
    (See documentation for details)

Copy link
Contributor

@abravalheri abravalheri left a comment

Choose a reason for hiding this comment

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

Thank you very much @Avasam.

Other than the open question of what to do with extern, I think all the other changes are good.

Comment on lines 109 to 113
def yield_lines(iterable: "_NestedStr") -> chain[str]: ...

# Trick type-checkers into seeing the original instances for type comparisons
from packaging import version as _packaging_version
from packaging import requirements as _packaging_requirements
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should defer any changes like this until we have a decision about how to reconcile extern and type-cheking.

if TYPE_CHECKING:
    # ...
    def yield_lines(iterable: "_NestedStr") -> chain[str]: ...

    # Trick type-checkers into seeing the original instances for type comparisons
    from packaging import version as _packaging_version
    from packaging import requirements as _packaging_requirements

Copy link
Contributor Author

@Avasam Avasam May 24, 2024

Choose a reason for hiding this comment

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

Even if we did, this part would be useful. At least the two imports.

Even if static type-checkers understood the extern imports, they would be treated as a different type than the originals:

from ._vendor.packaging.version import Version as parse_version
from packaging.version import Version

def foo(bar: Version): ...
foo(parse_version("0.0.0"))  # type error

Pyright:

error: Argument of type "Version" cannot be assigned to parameter "bar" of type "Version" in function "foo"
    "pkg_resources._vendor.packaging.version.Version" is incompatible with "packaging.version.Version" (reportArgumentType)

Mypy:

error: Argument 1 to "foo" has incompatible type "pkg_resources._vendor.packaging.version.Version"; expected "packaging.version.Version"  [arg-type]

The re-exported yield_lines method isn't typed either iirc. (Although that can be fixed upstream with a stub or TYPE_CHECKING block because of @functools.singledispatch)

So this is only related to the dynamic extern issue in that it would incidentally resolve part of it. But it's a different issue at its core that would still be present with statically vendored packages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(for the sake of this PR I'll split it off, but there's almost nothing left to do to mark pkg_resources as py.typed, so that's definitely something I'd like to handle as soon as all PRs linked in this PR's description are merged)

pkg_resources/__init__.py Outdated Show resolved Hide resolved
pkg_resources/__init__.py Outdated Show resolved Hide resolved
pkg_resources/__init__.py Outdated Show resolved Hide resolved
pkg_resources/__init__.py Outdated Show resolved Hide resolved
pkg_resources/__init__.py Outdated Show resolved Hide resolved
pkg_resources/__init__.py Outdated Show resolved Hide resolved
pkg_resources/__init__.py Outdated Show resolved Hide resolved
pkg_resources/__init__.py Outdated Show resolved Hide resolved
@Avasam Avasam force-pushed the fix-incorrect-implicit-return-types branch from 35c9174 to dc0a507 Compare June 17, 2024 17:53
@abravalheri abravalheri merged commit 8ac08a0 into pypa:main Jun 17, 2024
22 checks passed
@abravalheri
Copy link
Contributor

Thank you!

@Avasam Avasam deleted the fix-incorrect-implicit-return-types branch June 17, 2024 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants