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

Deduplicate pyright version #9299

Merged
merged 3 commits into from
Nov 30, 2022
Merged

Conversation

Avasam
Copy link
Sponsor Collaborator

@Avasam Avasam commented Nov 29, 2022

No one likes duplicated configs that have to be kept in sync.
Proposed solution: store it at a single location, then retrieve it when needed. This implementation also requires no parsing.
image

@srittau
Copy link
Collaborator

srittau commented Nov 29, 2022

+1 on the idea. But maybe we could add the pyright version to requirements-tests.txt as a comment?

@AlexWaygood
Copy link
Member

AlexWaygood commented Nov 29, 2022

I agree it's a great idea to clean this up. Here's an alternative implementation to consider, where we store the version in a tool.typeshed section in our pyproject.toml file: AlexWaygood@56bc7a8 (here's the CI run for that commit). We could potentially use this tool.typeshed table for other things in the future, such as listing the Python versions and Python platforms typeshed supports in our test suite.

@Avasam
Copy link
Sponsor Collaborator Author

Avasam commented Nov 29, 2022

I like the pyproject.toml location better. Thanks for the CI toml loader example.

We could potentially use this tool.typeshed table for other things in the future, such as listing the Python versions and Python platforms typeshed supports in our test suite.

Agreed, for instance all mypy scripts are run on at least the oldest non-eol python version. Some tests are done on 3.9+ because of annotations. And others just use the latest python version typeshed works with. Those ranges could be named and defined in a common config file.
Afaik it's possible to do dynamic matrixes in github actions. You just need to add a dependent job that checksout the repo and reads the config file

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
@srittau
Copy link
Collaborator

srittau commented Nov 29, 2022

I'd prefer to have our dependencies in one place and not split over multiple files.

@AlexWaygood
Copy link
Member

I'd prefer to have our dependencies in one place and not split over multiple files.

Fair, I get that. What would the requirements.txt solution look like?

@Avasam
Copy link
Sponsor Collaborator Author

Avasam commented Nov 29, 2022

I'd prefer to have our dependencies in one place and not split over multiple files.

Fair, I get that. What would the requirements.txt solution look like?

That's the thing. requirements*.txt is for pip requirements. Highjacking it feels like a hack.
And adding something like # pyright==1.1.278 could be confused with commenting out a https://pypi.org/project/pyright/ install. Parsing it may also be a bit more complex (?)

On the other hand, pyproject.toml is a good place for both project dependencies and tooling configurations. So it felt more natural to go there.

@srittau
Copy link
Collaborator

srittau commented Nov 30, 2022

I'd prefer to have our dependencies in one place and not split over multiple files.

Fair, I get that. What would the requirements.txt solution look like?

Put it in a comment in the same format as other dependencies. Alternatively, put all dependencies into pyproject, but I don't think this would work yet. Still, this PR is an improvement over the status quo, so I won't reject it, but I still believe splitting dependencies over multiple files is quite confusing.

@AlexWaygood
Copy link
Member

I still believe splitting dependencies over multiple files is quite confusing.

I do agree, but I also agree with @Avasam that putting it in requirements-tests.txt might be confusing for different reasons — it's a very different kind of dependency to the others, after all.

I'm going to merge this for now, as I think it's a strict improvement over the status quo, but I'm definitely open to further improvements in the future :) Agreed that putting all dependencies in pyproject.toml might be ideal, if it's possible.

@AlexWaygood AlexWaygood merged commit 5c3f10e into python:main Nov 30, 2022
@Avasam Avasam deleted the dedupe-pyright-version branch November 30, 2022 15:17
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.

4 participants