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.

Sign up for GitHub

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

Jump to bottom

Support external-dependencies in pyright #9434

Merged
merged 22 commits into from Jan 3, 2023
Merged

Support external-dependencies in pyright #9434

merged 22 commits into from Jan 3, 2023

Conversation

Avasam
Copy link
Sponsor Collaborator

@Avasam Avasam commented Dec 31, 2022 •
edited

Extracted from #9374
Work towards #5768

Support external (non-type) dependencies in pyright in the CI.
This also adds a new reusable script that can be run from CLI to get a list of dependencies (similar to get_packages.py) and leverages the read_dependencies method added in #9382

AlexWaygood
AlexWaygood reviewed Dec 31, 2022
View reviewed changes
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! A few nits:

.github/workflows/tests.yml Outdated Show resolved Hide resolved
.github/workflows/tests.yml Outdated Show resolved Hide resolved
tests/get_external_dependencies.py Outdated Show resolved Hide resolved
tests/get_packages.py Outdated Show resolved Hide resolved
@Avasam
Fix pyright dep installs
a7315e8 
- Install all from requirements-tests.txt
- lock pyright to specific version
- deduplicate dependencies from script
- remove extra space
@Avasam Avasam requested review from AlexWaygood and JelleZijlstra and removed request for AlexWaygood December 31, 2022 19:57
AlexWaygood
AlexWaygood reviewed Dec 31, 2022
View reviewed changes
tests/get_external_dependencies.py Outdated Show resolved Hide resolved
tests/utils.py Outdated Show resolved Hide resolved
Avasam
Avasam commented Dec 31, 2022
View reviewed changes
tests/utils.py Outdated Show resolved Hide resolved
AlexWaygood
AlexWaygood reviewed Jan 1, 2023
View reviewed changes
tests/utils.py Outdated Show resolved Hide resolved
@AlexWaygood
Copy link
Member

AlexWaygood commented Jan 1, 2023 •
edited

Presumably pyright_test.py won't work unless contributors have already run tests/get_external_dependencies.py in their venv first. Maybe we should add a note to that effect in tests/README.md and/or the docstring for pyright_test.py?

@AlexWaygood
Copy link
Member

Happy new year from the UK, by the way! 😀 🎆

@Avasam Avasam mentioned this pull request Jan 1, 2023
Merged
AlexWaygood
AlexWaygood reviewed Jan 1, 2023
View reviewed changes
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.

We're getting there! A few more comments below.

As well as these comments: I'm slightly concerned that the name of the new script (get_external_dependencies.py) is quite similar to the existing script get_packages.py. Can we disambiguate them somehow? Maybe we should rename get_packages.py, since that one has quite a vague name currently?

.github/workflows/tests.yml Outdated Show resolved Hide resolved
.github/workflows/tests.yml Show resolved Hide resolved
.github/workflows/tests.yml Show resolved Hide resolved
tests/README.md Outdated Show resolved Hide resolved
tests/README.md Outdated Show resolved Hide resolved
tests/README.md Outdated Show resolved Hide resolved
tests/README.md Outdated Show resolved Hide resolved
@Avasam
Copy link
Sponsor Collaborator Author

Avasam commented Jan 1, 2023

[...] I'm slightly concerned that the name of the new script (get_external_dependencies.py) is quite similar to the existing script get_packages.py. Can we disambiguate them somehow? Maybe we should rename get_packages.py, since that one has quite a vague name currently?

How about something like get_system_packages and get_external_[python]_requirements ? Name is a bit long, but tab-completion is also a thing for CLI.

@AlexWaygood
Copy link
Member

How about something like get_system_packages and get_external_[python]_requirements ? Name is a bit long, but tab-completion is also a thing for CLI.

Yeah, those sound great to me! 👍

@Avasam @AlexWaygood
Apply suggestions from code review
8dd53a2 
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
@AlexWaygood
Copy link
Member

AlexWaygood commented Jan 1, 2023 •
edited

Actually, maybe something like get_stubtest_system_requirements.py and get_external_stub_requirements.py? That might clarify that the first script doesn't install requirements of the stubs package, it just installs requirements stubtest needs, in contrast to the second script.

What do you think?

@AlexWaygood AlexWaygood mentioned this pull request Jan 2, 2023
Closed
Avasam
Avasam commented Jan 2, 2023
View reviewed changes
tests/README.md Outdated Show resolved Hide resolved
AlexWaygood
AlexWaygood reviewed Jan 2, 2023
View reviewed changes
tests/README.md Outdated Show resolved Hide resolved
@Avasam @AlexWaygood
Update tests/README.md
1ff9c18 
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
AlexWaygood
AlexWaygood reviewed Jan 2, 2023
View reviewed changes
tests/README.md Outdated Show resolved Hide resolved
AlexWaygood
AlexWaygood approved these changes Jan 2, 2023
View reviewed changes
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, other than the issue about the names of the files themselves :)

@Avasam Avasam mentioned this pull request Jan 2, 2023
Merged
@Avasam
Copy link
Sponsor Collaborator Author

Avasam commented Jan 2, 2023

Actually, maybe something like get_stubtest_system_requirements.py and get_external_stub_requirements.py? That might clarify that the first script doesn't install requirements of the stubs package, it just installs requirements stubtest needs, in contrast to the second script.

What do you think?

SGTM. I also fixed-up a comment.

@Avasam Avasam mentioned this pull request Jan 3, 2023
Merged
@AlexWaygood AlexWaygood force-pushed the support-non-type-dependency-pyright branch from 69974ae to b5a2495 Compare January 3, 2023 22:08
@AlexWaygood
Copy link
Member

(Sorry for the noise, just doing some final testing!)

AlexWaygood
AlexWaygood approved these changes Jan 3, 2023
View reviewed changes
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.

Looks great! Will merge once CI passes. Thanks for working on this!

@github-actions

This comment was marked as outdated.

Sign in to view
@AlexWaygood AlexWaygood merged commit 3e24c65 into python:main Jan 3, 2023
@Avasam Avasam deleted the support-non-type-dependency-pyright branch January 3, 2023 23:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlexWaygood AlexWaygood AlexWaygood approved these changes

@JelleZijlstra JelleZijlstra Awaiting requested review from JelleZijlstra

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Avasam @AlexWaygood @JelleZijlstra