FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

type annotations by dimbleby · Pull Request #449 · python/importlib_metadata · GitHub

/ importlib_metadata Public

type annotations - #449

Merged
jaraco merged 4 commits into
python:mainfrom
dimbleby:type-annotations
Apr 22, 2023
ConversationCommits (4)ChecksFiles changed
Merged

type annotations#449
jaraco merged 4 commits into
python:mainfrom
dimbleby:type-annotations

Conversation

dimbleby commented Apr 15, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

It's kind of annoying that this library publishes a py.typed, but most of the API is not type-annotated. Users who check their own code with mypy are obliged to scatter around # type: ignore[no-untyped-call] comments.

Possible points of interest:

  • joinpath on the SimplePath seems to have been just wrong
  • I've annotated some Iterables as Iterators, because (i) they are and (ii) Distribution.from_name() relies on this when it calls next(..) rather than next(iter(..))

This MR doesn't annotate the whole API: I've ducked the slightly difficult ones like distributions(), select() and matches(). typeshed has annotations that are presumably satisfactory in practice, but it looks as though applying them here would be more invasive than I intend to be in this commit. https://github.com/python/typeshed/blob/main/stdlib/importlib/metadata/__init__.pyi

This MR is a long way from annotating the whole project - mypy --strict importlib_metadata reports 171 errors, so it would take a bit more of a campaign to work through that.

jaraco commented Apr 15, 2023

Copy link
Copy Markdown
Member

FYI, @RonnyPfannschmidt was also considering tackling this effort (discussion in #importlib-metadata channel on PyPA discord).

dimbleby commented Apr 15, 2023

Copy link
Copy Markdown
Contributor Author

Cool. I'm mostly just aiming for the low-hanging fruit here and perhaps testing the waters for receptiveness to further work.

The pipeline and I both are currently happy; I reckon I'm done fiddling with this one now.

RonnyPfannschmidt commented Apr 15, 2023

Copy link
Copy Markdown
Contributor

I have not yet started own work, great to see this start

jaraco reviewed Apr 18, 2023
View reviewed changes

jaraco left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This looks good and nearly ready to merge. Just a few concerns to address.

Comment thread importlib_metadata/__init__.py Outdated
Comment thread importlib_metadata/__init__.py Outdated
Comment thread importlib_metadata/__init__.py Outdated
Comment thread importlib_metadata/__init__.py Outdated
Comment thread importlib_metadata/__init__.py Outdated
Comment thread importlib_metadata/__init__.py Outdated
Comment thread importlib_metadata/__init__.py Outdated

dimbleby commented Apr 18, 2023

Copy link
Copy Markdown
Contributor Author

OK, I've pushed change that - I hope - err in the direction of avoiding and deferring any difficult questions

  • prefer Iterable to Iterator (per the docstrings, and leaving other or future implementations a little more freedom)

    • as a result, added an iter() call in a place that was assuming it had an iterator
    • actually I notice now that I've annotated the requires() methods as returning List where the docstring used to say "iterator" (definitely wrong) and after my search-replace says "iterable" (true, but less specific than list). Any preference on doing anything about that?

  • completely ducked the typing of Sectioned.valid(), I don't like either of our suggestions but don't want to get stuck on that

  • updated StrPath as suggested

  • have Distribution.at() return Distributions rather than committing to PathDistributions

    • just spotted MetadataPathFinder.find_distributions() - is it ok for that to commit to PathDistribution or does that want changing too?

jaraco commented Apr 22, 2023

Copy link
Copy Markdown
Member
  • just spotted MetadataPathFinder.find_distributions() - is it ok for that to commit to PathDistribution or does that want changing too?

Yes. Distribution is the right choice here. Thanks.

jaraco merged commit 705a757 into python:main Apr 22, 2023

dimbleby commented Apr 22, 2023

Copy link
Copy Markdown
Contributor Author

Yes. Distribution is the right choice here. Thanks.

the code that you have merged still used PathDistribution:

importlib_metadata/importlib_metadata/__init__.py

Lines 796 to 798 in 705a757

def find_distributions(
self, context=DistributionFinder.Context()
) -> Iterable["PathDistribution"]:

dimbleby deleted the type-annotations branch April 22, 2023 12:43

jaraco commented Apr 22, 2023

Copy link
Copy Markdown
Member

Yes. Distribution is the right choice here. Thanks.

the code that you have merged still used PathDistribution:

Aah. I was looking at the wrong thing (because it wasn't in the diff). I think PathDistribution is okay here because it's a specific implementation that does specifically only return PathDistribution (even though other DistributionFinders might return a more generic Distribution). Thanks for flagging it.

jaraco commented Dec 16, 2023

Copy link
Copy Markdown
Member

I realized that the declaration of Distribution.locate_file is incorrect and have started work to correct it in #480.

hroncok mentioned this pull request May 14, 2024
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jaraco jaraco left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL