Skip to content

Conversation

@henryiii
Copy link
Contributor

This was an attempt to fill out more of the typing for dist. I've re-synced setuptools/_distutils/dist.py with distutils/dist.py, as the vendored version was missing a lot of info from the other one.

@github-actions

This comment has been minimized.

Signed-off-by: Henry Schreiner <[email protected]>
@henryiii
Copy link
Contributor Author

henryiii commented Mar 16, 2023

Is it a good idea to include something in a stub if it's identical type-wise to the base class but it does exist in the subclass? That is:

class A:
    def f(self):
        return None

class B(A):
    def f(self):
       return super().f()

Should that be:

class A:
    def f(self) -> None: ...

class B(A):
    def f(self) -> None: ...

Or

class A:
    def f(self) -> None: ...

class B(A): ...

? This happens quite a bit in setuptool's Distribution.

I can push the fix for the mistake but this would help in selecting the right fix - repeating the correct type hint or removing the repeated type hints.

@AlexWaygood
Copy link
Member

Is it a good idea to include something in a stub if it's identical type-wise to the base class? That is:

We generally omit methods on the subclass if they have identical signatures to the methods on the superclass. But there are a few exceptions (mostly for dunders) — e.g. __eq__ and __format__ should always be included on the subclass, for Reasons

@AlexWaygood
Copy link
Member

AlexWaygood commented Mar 16, 2023

But if the signature differs on the subclass, you should definitely include it. If it's incompatible with the superclass, you should add a type: ignore: it's not our job to ensure that the runtime adheres to LSP :)

Our job is to faithfully represent what happens at runtime.

@henryiii
Copy link
Contributor Author

henryiii commented Mar 16, 2023

Is the stubtest taking into account side effects of running __init__? It seems like it has to, but it's claiming the generated methods (https://github.com/pypa/setuptools/blob/be6c0218bcba78dbd4ea0b5a8bb9acd5d5306240/setuptools/_distutils/dist.py#L151-L153) don't exist at runtime. Also https://github.com/python/cpython/blob/1bf9cc509326bc42cd8cb1650eb9bf64550d817e/Lib/distutils/dist.py#L159-L162.

I can pull this out to a separate PR.

@AlexWaygood
Copy link
Member

Stubtest works by importing the module at runtime and dynamically inspecting the objects it finds in it, comparing the results to what it sees in the stub. So it often struggles with methods generated inside __init__ methods, since it never actually instantiates any of the classes at runtime :)

If need be, we will just have to add allowlist entries to tell it to be quiet about these methods.

(Note: I haven't studied your proposed changes here yet; this is just a direct response to the question you posed above :)

@henryiii
Copy link
Contributor Author

Okay, that would explain it. I'd have expected something simple like assigning inside an init method to also have a problem (thought that's not usually making a method, perhaps it's only checking methods?) Anyway, I've pulled that out separately, as I think it's easier to review on it's own.

@AlexWaygood
Copy link
Member

AlexWaygood commented Mar 16, 2023

perhaps it's only checking methods?

Yup, 99% of methods are statically defined, so if there's a method in the stub that it can't see a method at runtime, stubtest assumes you've made a mistake and emits an error. But 99% of instance attributes are assigned in __init__ methods, so if there's an instance attribute in the stub that it can't see at runtime, it generally won't emit an error.

Stubtest is basically just a series of hardcoded consistency checks; it doesn't have a generalised way of comparing the runtime to the stub. But it's very effective at what it does (it just has a few edge cases, and you've stumbled on one of them!).

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@henryiii
Copy link
Contributor Author

distutils.core.Distribution.get_command_obj is inconsistent, runtime argument "create" has a default value of 1, which is different from stub argument default True

Is the there a way to silence that? Or should I just use ... for that one? It's a bool argument but it using 1, probably for Reasons. :)

Signed-off-by: Henry Schreiner <[email protected]>
Signed-off-by: Henry Schreiner <[email protected]>
Signed-off-by: Henry Schreiner <[email protected]>
@github-actions

This comment has been minimized.

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Thanks! Haven't looked in depth at the setuptools changes yet, but here's a few points on the distutils changes -- I think a few of them will also apply to some of the setuptools changes

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

henryiii and others added 2 commits March 17, 2023 10:19
@github-actions

This comment has been minimized.

Signed-off-by: Henry Schreiner <[email protected]>
Signed-off-by: Henry Schreiner <[email protected]>

refactor: use overload for reinit command

Signed-off-by: Henry Schreiner <[email protected]>
@github-actions

This comment has been minimized.

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Thanks, nearly there! A few more nits from me:

@github-actions

This comment has been minimized.

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@github-actions
Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

sphinx (https://github.com/sphinx-doc/sphinx)
- sphinx/setup_command.py: note: In member "initialize_options" of class "BuildDoc":
- sphinx/setup_command.py:107:26: error: "Distribution" has no attribute "verbose"  [attr-defined]

isort (https://github.com/pycqa/isort)
- isort/setuptools_commands.py:34: error: "Distribution" has no attribute "packages"  [attr-defined]
- isort/setuptools_commands.py:35: error: "Distribution" has no attribute "package_dir"  [attr-defined]
- isort/setuptools_commands.py:36: error: "Distribution" has no attribute "packages"  [attr-defined]
- isort/setuptools_commands.py:44: error: "Distribution" has no attribute "py_modules"  [attr-defined]
- isort/setuptools_commands.py:45: error: "Distribution" has no attribute "py_modules"  [attr-defined]

@AlexWaygood AlexWaygood merged commit 2d990ee into python:main Mar 17, 2023
@henryiii henryiii deleted the henryiii/chore/moresetuptools branch March 17, 2023 19:04
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.

3 participants