Skip to content

Conversation

@Avasam
Copy link
Collaborator

@Avasam Avasam commented Nov 29, 2022

No description provided.

@Avasam
Copy link
Collaborator Author

Avasam commented Nov 29, 2022

I meant to open against my own fork to validate what kindof changes would be necessary and if worth

@Avasam Avasam closed this Nov 29, 2022
Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

We should definitely make sure pyright runs against test_cases, but don't think it's particularly important for pyright to run against the scripts or the testing code. I'm not seeing any real issues caught here, so mostly seems like it'd cause busywork for future contributions.

Edit: never mind, saw your edit :-)

@Avasam
Copy link
Collaborator Author

Avasam commented Nov 29, 2022

@hauntsaninja It did allow me to find a couple typing issues with libraries used by typeshed (namely pathspec, tomlkit and yaml), and two (maybe three, to validate) issues with pyright itself.
Fixing those would improve typeshed contributors experience by providing more/better type hints. Bu it's outside typeshed itself.

It also made me realize parsing METADATA.toml could be improved by parsing it in a central location, and returning a TypedDict. That way the default values are constant (no need to re-implement everytime), no need to revalidate keys everytime, no need to constantly do .get and type-hints will be provided.
There's also a couple of keys and values that are not validated rn.

So even if in the end checking scripts and testing with pyright is not something that's done, it will have been a fruitful exercice for me.

@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Nov 29, 2022

Yeah, agreed about METADATA.toml. In stub_uploader this is what I went with: https://github.com/typeshed-internal/stub_uploader/blob/main/stub_uploader/metadata.py Might be nice to have something like that in typeshed too

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.

2 participants