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

Add type stub file for setup #2464

Closed
wants to merge 6 commits into from
Closed

Add type stub file for setup #2464

wants to merge 6 commits into from

Conversation

GergelyKalmar
Copy link

@GergelyKalmar GergelyKalmar commented Nov 28, 2020

Summary of changes

This PR adds a stub file for the setup() function. I've been mostly relying on the setuptools keywords documentation to derive the type hints. I left out the deprecated arguments and the arguments related to Python 2 (the catch-all argument will allow using these anyways). I also marked version optional mostly because it can be left out when using tools like setuptools_scm.

I'm not quite sure how to write tests for module stub files, so suggestions / additions in this area are welcome.

Closes #2345

Pull Request Checklist

  • Changes have tests
  • News fragment added in changelog.d

@webknjaz
Copy link
Member

Looks like the failing test is unrelated:

stdout = b"/home/runner/work/setuptools/setuptools/.tox/python/bin/python: can't open file 'setup.py': [Errno 2] No such file or directory\n"

Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

I like the idea of having the type hints but I think that the implementation should be slightly different: there's no need for separate stub files, just add the typing info right into https://github.com/pypa/setuptools/blob/5cf3865/setuptools/__init__.py#L150 because setuptools requires Python 3.6 (https://github.com/pypa/setuptools/blob/5cf3865/setup.cfg#L48)

@GergelyKalmar
Copy link
Author

@webknjaz Thanks for the feedback! We could do that, but it would be a bit less elegant in my opinion, mostly because https://github.com/pypa/setuptools/blob/5cf3865/setuptools/__init__.py#L150 forwards its arguments to both _install_setup_requires and distutils.core.setup, which means that we would need to list the arguments three times (instead of having them just listed once in the stub and using **attrs in the actual function). I'm not sure if there's an elegant workaround for this, but if there isn't perhaps maintaining this in the stub file is not the worst idea.

The tests do seem to fail on the plain master branch too, I think they're unrelated. We could add mypy and write some test files to be type-checked though for sure, but there's perhaps a more elegant way to do this too.

@webknjaz
Copy link
Member

I think that keeping a separate stub in sync is harder than having it copy-pasted in code. Also, the copy-paste could be partially eliminated with a decorator if you think that it's that critical.
Anyway, it's up to @jaraco to decide. You should probably wait for his opinion to land before doing anything.

P.S. It's more important to work on making master green now. To eliminate the misleading PR statuses.

@GergelyKalmar
Copy link
Author

It's fine for me either way – I've chosen this path because that's what was suggested in the original issue discussion. I'm happy to change if @jaraco thinks it's worthwhile.

@jaraco jaraco closed this Dec 12, 2020
@jaraco jaraco reopened this Dec 12, 2020
Copy link
Member

@jaraco jaraco left a comment

Choose a reason for hiding this comment

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

We could add mypy and write some test files to be type-checked though for sure, but there's perhaps a more elegant way to do this too.

In the feature/skeleton branch, I'm planning to merge jaraco/skeleton into this project, which adds mypy support. Perhaps it would be worth landing that before landing this? Or you could merge that branch with this one and test the two together.

It's more important to work on making master green now. To eliminate the misleading PR statuses.

Agreed. And done in #2472.

We could [use inline annotations], but it would be a bit less elegant in my opinion, mostly because https://github.com/pypa/setuptools/blob/5cf3865/setuptools/__init__.py#L150 forwards its arguments to both _install_setup_requires and distutils.core.setup, which means that we would need to list the arguments three times (instead of having them just listed once in the stub and using **attrs in the actual function).

I also agree it would be preferable to use inline annotations. If there are concerns about the underlying implementation (usage), that seems problematic for the typing implementation. I'd be okay with having a wrapper for setup, e.g.:

def setup(*, name: str, ...):
    attrs = ...
    _setup(**attrs)

As I think about it more, I'm not sure it's even possible to declare accurately the types for this function. It might be that one would need to specify a PEP 589 TypedDict or similar.

That said, I'm okay with a .pyi file for now and transition to an inline definition as the type system allows.

setuptools/__init__.pyi Outdated Show resolved Hide resolved
setuptools/__init__.pyi Outdated Show resolved Hide resolved
setuptools/__init__.pyi Outdated Show resolved Hide resolved
setuptools/__init__.pyi Outdated Show resolved Hide resolved
@jaraco jaraco changed the base branch from master to main December 12, 2020 17:16
@GergelyKalmar
Copy link
Author

We could [use inline annotations], but it would be a bit less elegant in my opinion, mostly because https://github.com/pypa/setuptools/blob/5cf3865/setuptools/__init__.py#L150 forwards its arguments to both _install_setup_requires and distutils.core.setup, which means that we would need to list the arguments three times (instead of having them just listed once in the stub and using **attrs in the actual function).

I also agree it would be preferable to use inline annotations. If there are concerns about the underlying implementation (usage), that seems problematic for the typing implementation. I'd be okay with having a wrapper for setup, e.g.:

def setup(*, name: str, ...):
    attrs = ...
    _setup(**attrs)

As I think about it more, I'm not sure it's even possible to declare accurately the types for this function. It might be that one would need to specify a PEP 589 TypedDict or similar.

Hm, that would possibly be the most accurate option. Wouldn't that work only with Python 3.8+ though?

@GergelyKalmar
Copy link
Author

With regards to the mypy support, I can surely merge that branch in too. Is it ready to be integrated?

@jaraco
Copy link
Member

jaraco commented Dec 14, 2020

With regards to the mypy support, I can surely merge that branch in too. Is it ready to be integrated?

It’s not ready to be integrated with main or this PR. I was suggesting to merge temporarily or to another branch to test.

@jaraco
Copy link
Member

jaraco commented May 23, 2021

The mypy support is now integrated, but not enabled. Running the tests with tox -- --mypy will run the tests, but the code has many failures. I'm not sure how helpful the plugin will be for this effort until the failures are addressed.

@webknjaz
Copy link
Member

Time to rebase or close?

@jaraco jaraco closed this Jul 17, 2021
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.

Add type hints to setuptools
3 participants