Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

type annotations#449

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
jaraco merged 4 commits intopython:mainfromdimbleby:type-annotations
Apr 22, 2023
Merged

type annotations#449

jaraco merged 4 commits intopython:mainfromdimbleby:type-annotations
Apr 22, 2023

Conversation

@dimbleby
Copy link
Contributor

@dimblebydimbleby commentedApr 15, 2023•
edited
Loading

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 theSimplePath seems to have been just wrong
  • I've annotated someIterables asIterators, because (i) they are and (ii)Distribution.from_name() relies on this when it callsnext(..) rather thannext(iter(..))

This MR doesn't annotate the whole API: I've ducked the slightly difficult ones likedistributions(),select() andmatches(). 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
Copy link
Member

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

@dimbleby
Copy link
ContributorAuthor

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
Copy link
Contributor

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

Copy link
Member

@jaracojaraco left a comment

Choose a reason for hiding this comment

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

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

@dimbleby
Copy link
ContributorAuthor

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

  • preferIterable toIterator (per the docstrings, and leaving other or future implementations a little more freedom)

    • as a result, added aniter() call in a place that was assuming it had an iterator
    • actually I notice now that I've annotated therequires() methods as returningList 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 ofSectioned.valid(), I don't like either of our suggestions but don't want to get stuck on that

  • updatedStrPath as suggested

  • haveDistribution.at() returnDistributions rather than committing toPathDistributions

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

@jaraco
Copy link
Member

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

Yes.Distribution is the right choice here. Thanks.

@jaracojaraco merged commit705a757 intopython:mainApr 22, 2023
@dimbleby
Copy link
ContributorAuthor

Yes. Distribution is the right choice here. Thanks.

the code that you have merged still usedPathDistribution:

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

@dimblebydimbleby deleted the type-annotations branchApril 22, 2023 12:43
@jaraco
Copy link
Member

Yes. Distribution is the right choice here. Thanks.

the code that you have merged still usedPathDistribution:

Aah. I was looking at the wrong thing (because it wasn't in the diff). I thinkPathDistribution is okay here because it's a specific implementation that does specifically only returnPathDistribution (even though otherDistributionFinders might return a more genericDistribution). Thanks for flagging it.

dimbleby reacted with thumbs up emoji

@jaraco
Copy link
Member

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

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@jaracojaracojaraco 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

@dimbleby@jaraco@RonnyPfannschmidt
[8]ページ先頭
©2009-2025 Movatter.jp