Skip to content

Conversation

@Avasam
Copy link
Contributor

@Avasam Avasam commented Oct 28, 2024

Summary of changes

I commented [lint.per-file-ignores] "setuptools/**" = ["ANN2"] and [lint.flake8-annotations] ignore-fully-untyped = true in ruff.toml then ran ruff check setuptools --select=ANN201 --fix --unsafe-fixes.
I removed the added return types that were revealing hidden mypy issues (often initialize_options) to keep the changes here completely automated.

This has some overlap with #4709, but they can be merged in either order.

Pull Request Checklist

  • Changes have tests (this increased the amount of type-checked code)
  • News fragment added in newsfragments/. (no public facing changes)
    (See documentation for details)

@Avasam Avasam mentioned this pull request Oct 28, 2024
2 tasks
@Avasam Avasam changed the title setuptools ANN201 autofixes setuptools ANN201 autofixes for fully untyped functions Oct 28, 2024
@Avasam Avasam force-pushed the setuptools-ANN201-autofixes branch 2 times, most recently from 59376d7 to 6d830ef Compare October 30, 2024 18:00

@property
def parsers(self):
def parsers(self) -> NoReturn:
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, isn't this going to make more complicated inheritance and require some form of overload annotation later?

It is kind of an abstract method that needs to be implemented.
The expected signature is something like () -> dict[str, Callable[[str], Any]] or more specific1.
I suppose that for the sake of documentation/understanding that return value as a dict is better than NoReturn.

Footnotes

  1. The return values for the callable are probably something like all the types that can be produced by ast.parse_literal (or subtypes) + packaging.specifiers.SpecifierSet

Copy link
Contributor Author

@Avasam Avasam Oct 31, 2024

Choose a reason for hiding this comment

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

NoReturn is a bottom type, so as far as subclassing it shouldn't be an issue.

For consumers it means that if something returns a ConfigHandler, they'd see accessing the property parsers as never returning (unless they use isinstance to ensure a subtype or something).

So you are correct that

I suppose that for the sake of documentation/understanding that return value as a dict is better than NoReturn.

Even better, this property should be marked abstract, so type-checkers tells the dev to never instanciate ConfigHandler directly, which implies that any ConfigHandler the dev gets should be a concrete subclass, so returning NoReturn for a NotImplementedError is no longer relevant.

But that's for a separate PR ! (I'll just revert this line here)

@Avasam Avasam force-pushed the setuptools-ANN201-autofixes branch from 6d830ef to 747ae28 Compare October 31, 2024 14:15
@Avasam Avasam force-pushed the setuptools-ANN201-autofixes branch from 747ae28 to 04b515e Compare October 31, 2024 14:35
@abravalheri abravalheri merged commit dc2050e into pypa:main Oct 31, 2024
21 of 23 checks passed
@abravalheri
Copy link
Contributor

Thank you very much.

@Avasam Avasam deleted the setuptools-ANN201-autofixes branch October 31, 2024 21:25
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