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

Merged
merged 18 commits into from
Oct 25, 2024

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 ?

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 can be a bit bold here and replace it directly... people should not be importing StrIter from this module... A search on GitHub seems to indicate they are not: https://github.com/search?q=%2Fsetuptools.discovery.*StrIter%2F&type=code.

(the whole module should have been introduced as a private module... but at that point in time I did not know better).

setuptools/command/sdist.py Outdated Show resolved Hide resolved
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 ready for review October 17, 2024 16:24
# Installation is also needed if file in tmpdir or is not an egg
install_needed = install_needed or self.always_copy
install_needed = install_needed or bool(self.always_copy)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Either this, or:

  • default always_copy to False (I wasn't sure if None was used as a special meaning "not set by user");
  • allow install_needed param to be None;
  • use a differently named variable in install_item (like _install_needed);

@Avasam Avasam mentioned this pull request Oct 17, 2024
1 task
@Avasam
Copy link
Contributor Author

Avasam commented Oct 21, 2024

I've added more distutils workaround so that this can be merged w/o #4691, but I'd ask to consider making distutils stubs available for all Python versions so that it stops being an issue.

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.

I added 2 more questions, but I think we can merge the PR as it is, after we apply the rebase and test. Worst case scenario we can have follow ups.

if TYPE_CHECKING:
from setuptools import Distribution
from typing_extensions import TypeAlias

StrIter: TypeAlias = Iterator[str]
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 can be a bit bold here and replace it directly... people should not be importing StrIter from this module... A search on GitHub seems to indicate they are not: https://github.com/search?q=%2Fsetuptools.discovery.*StrIter%2F&type=code.

(the whole module should have been introduced as a private module... but at that point in time I did not know better).

preserve_mode: bool = True,
preserve_times: bool = True,
link: str | None = None,
level: object = 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

        level: object = 1,

This one looks odd, but I suppose that is because it assumes either int or boolean values elsewhere.

Copy link
Contributor Author

@Avasam Avasam Oct 25, 2024

Choose a reason for hiding this comment

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

object can be used to mean "any parameter type" for unused params, (Typeshed has the alias _typeshed.Unused), but that might lead to LSP violations since subclasses should now support any object.

I should maybe change it to int (Unless you tell me that subclasses of these classes should not support an "optimization level" a depth level on this method)
image
Edit: uh that probably represent a depth level, not optimization ^^" Anyway, it's currently unused all the way down to the base Command class in distutils.

Copy link
Contributor Author

@Avasam Avasam Oct 25, 2024

Choose a reason for hiding this comment

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

I'll need to go fix it in typeshed first (or #4689) I'll leave it "as-is" for this PR. You can squash-merge the whole thing once green.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you @Avasam, once the CI finishes running I will squash it.

@abravalheri
Copy link
Contributor

So there was a problem in the tests with the latest version of ruff, and clash with the version of ruff in the .pre-commit-hooks.yaml.

I bumped the hook version just to keep the ball rolling. Hopefully it will not clash in the next merge with skeleton.

@abravalheri abravalheri force-pushed the setuptools-simple-typeshed-params branch from c8756df to 35e3ed4 Compare October 25, 2024 16:26
@Avasam
Copy link
Contributor Author

Avasam commented Oct 25, 2024

Ah yes, the formatter just added merging of short multiline implicit string concatenation in preview mode (along with a few other small changes).

Might be worth a standalone run on main, as to not "pollute" this PR with unrelated changes.

@abravalheri
Copy link
Contributor

Might be worth a standalone run on main, as to not "pollute" this PR with unrelated changes.

WIP in #4705.

@abravalheri abravalheri force-pushed the setuptools-simple-typeshed-params branch from 35e3ed4 to 96453cb Compare October 25, 2024 17:20
@abravalheri abravalheri merged commit 0bc3248 into pypa:main Oct 25, 2024
25 checks passed
@Avasam
Copy link
Contributor Author

Avasam commented Oct 25, 2024

Wooh! A big one done!

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