Skip to content

3rd-party stubtest: use MYPYPATH for internal dependencies #9512

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

Merged
merged 1 commit into from
Jan 12, 2023

Conversation

AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented Jan 12, 2023

Currently, if we're running stubtest on our stubs for caldav, we pip install all the requirements for types-caldav into the virtual environment before running the test. However, for typeshed-internal requirements, this is unnecessary: we can add the typeshed-internal requirements for caldav to MYPYPATH instead. This has two advantages:

  • pip installing stuff is slow; this should be faster. Some of our stubs have a lot of internal dependencies: when testing caldav, we currently have to pip install types-requests, types-urllib3 and types-vobject.
  • Currently it's impossible to e.g. add two stubs packages in one pull request, with one depending on the other. Instead, you have to add stubs package A, wait for the stub-uploader to push the new stubs to PyPI, and then add stubs package B, which depends on stubs package A. This has tripped up contributors in the past. With this update, we won't need to wait around for the stub-uploader.

@hauntsaninja hauntsaninja merged commit 00d663e into python:main Jan 12, 2023
@AlexWaygood AlexWaygood deleted the faster-stubtest branch January 12, 2023 21:14

return True


def print_commands(dist: Path, pip_cmd: list[str], stubtest_cmd: list[str]) -> None:
def print_commands(dist: Path, pip_cmd: list[str], stubtest_cmd: list[str], mypypath: str) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

@AlexWaygood The dist argument after this change has been left unused. Was that intentional ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope! It makes sense to get rid of it :D

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