-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Conversation
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" |
There was a problem hiding this 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)
@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 The tests do seem to fail on the plain master branch too, I think they're unrelated. We could add |
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. P.S. It's more important to work on making master green now. To eliminate the misleading PR statuses. |
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. |
There was a problem hiding this 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
anddistutils.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.
Take over changes
Hm, that would possibly be the most accurate option. Wouldn't that work only with Python 3.8+ though? |
With regards to the |
It’s not ready to be integrated with main or this PR. I was suggesting to merge temporarily or to another branch to test. |
The mypy support is now integrated, but not enabled. Running the tests with |
Time to rebase or close? |
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 markedversion
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
changelog.d