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

Merge simple type annotations from typeshed #4504

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Avasam
Copy link
Contributor

@Avasam Avasam commented Jul 22, 2024

Summary of changes

Please review and merge first: #4505 Done
Merging #4581 and #4584 first will reduce changes in this PR Done
It's important to validate this PR with mypy, let's re-enable it first and fix existing failures: #4604

Not done in this PR:

Step 6.1 of #2345 (comment)

Pull Request Checklist

  • Changes have tests (no new tests, existing static tests should pass)
  • News fragment added in newsfragments/. (nothing user-facing as static typing users would still be using types-setuptools)
    (See documentation for details)

@Avasam Avasam changed the title [WIP] Merge simple type annotations from typeshed Merge simple type annotations from typeshed Jul 22, 2024
Comment on lines 130 to 150
def __init__(
self, name: str, sources: list[str], *args, py_limited_api: bool = False, **kw
):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

From @abravalheri https://github.com/pypa/setuptools/pull/4505/files#r1709465928

Jason might have added support for PathLikesources recently in pypa/distutils#237 (v72.2.0/setuptools/_distutils/extension.py#L29).

(PS.: granted they are incompatible with SETUPTOOLS_USE_DISTUTILS=stdlib but that is a deprecated code path).

_distutils_hack/__init__.py Outdated Show resolved Hide resolved
@Avasam Avasam marked this pull request as ready for review August 8, 2024 19:10
_distutils_hack/__init__.py Outdated Show resolved Hide resolved
setuptools/build_meta.py Outdated Show resolved Hide resolved
@@ -289,7 +297,7 @@ def _arbitrary_args(self, config_settings: _ConfigSettings) -> Iterator[str]:


class _BuildMetaBackend(_ConfigSettingsTranslator):
def _get_build_requires(self, config_settings, requirements):
def _get_build_requires(self, config_settings: _ConfigSettings, requirements):
Copy link
Contributor

Choose a reason for hiding this comment

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

It is likely here that requirements and the return value are both list[str]. Not sure how the type-checking would react to that though...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesn't cause any issue. I didn't add annotations to _get_build_requires because it's not typed in typeshed. config_settings param being typed is incidental to adding all _ConfigSettings.

But since you're mentioning it here and it's not an issue, I've added the type annotation to requirements param

setuptools/warnings.py Outdated Show resolved Hide resolved
setuptools/sandbox.py Show resolved Hide resolved
setuptools/dist.py Show resolved Hide resolved
setuptools/extension.py Outdated Show resolved Hide resolved
setuptools/package_index.py Show resolved Hide resolved
setuptools/dist.py Show resolved Hide resolved
setuptools/__init__.py Outdated Show resolved Hide resolved
@Avasam
Copy link
Contributor Author

Avasam commented Aug 9, 2024

@abravalheri I'll answer a couple comments at once:

  • I have a handful of TypeAliases unnecessarily duplicated behind TYPE_CHECKING check whilst a from __future__ import annotations is sufficient to use an annotation that doesn't exist at runtime. So those should be simplified now.
  • As quickly mentioned in the PR description, and described in my step-by-step plan in Add type hints to setuptools #2345 (comment), I purposefully avoided adding return type annotations here. I plan on merging return annotations from typeshed as a direct follow-up. Then re-enabling ANN2 checks for setuptools.

@Avasam Avasam requested a review from abravalheri August 9, 2024 17:08
setuptools/__init__.py Outdated Show resolved Hide resolved
setuptools/build_meta.py Outdated Show resolved Hide resolved
setuptools/build_meta.py Outdated Show resolved Hide resolved
if TYPE_CHECKING:
from setuptools import Distribution
from typing_extensions import TypeAlias

StrIter: TypeAlias = Iterator[str]
Copy link
Contributor Author

@Avasam Avasam Aug 9, 2024

Choose a reason for hiding this comment

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

I don't feel like this alias brings anything of value:

  • It doesn't work around any runtime issue
  • It doesn't abstract away any complexity
  • It's not clearer than the type it's aliasing, or doesn't help explain it
  • It's barely any more concise (because it's using a shorthand instead of being clear and explicit)

In fact, it might actually hurt understandability as a dev can reasonably assume that the alias might be abstracting a more complex type and have to check what it is, rather than immediately understanding it's Iterator[str]

But it's not private. Could we maybe deprecate it ?

setuptools/command/sdist.py Outdated Show resolved Hide resolved
@Avasam Avasam force-pushed the setuptools-simple-typeshed-params branch from 8f34df1 to f319cc5 Compare August 20, 2024 18:44
@Avasam Avasam force-pushed the setuptools-simple-typeshed-params branch from f319cc5 to d4143c0 Compare August 20, 2024 21:44
@Avasam Avasam mentioned this pull request Aug 21, 2024
2 tasks
@Avasam Avasam force-pushed the setuptools-simple-typeshed-params branch 2 times, most recently from 9e0cc8d to 3113380 Compare August 27, 2024 15:27
@Avasam Avasam force-pushed the setuptools-simple-typeshed-params branch from 3113380 to 4eef53a Compare August 27, 2024 15:58
@Avasam Avasam marked this pull request as draft August 28, 2024 01:36
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