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

Fix unnecessary 'pyright: ignore' warning in CI #13101

Closed
wants to merge 6 commits into from

Conversation

srittau
Copy link
Collaborator

@srittau srittau commented Nov 25, 2024

No description provided.

This comment has been minimized.

This comment has been minimized.

@srittau srittau marked this pull request as ready for review November 25, 2024 16:30
@AlexWaygood
Copy link
Member

Hmm, what problem is this fixing? I see lots of stdlib stubtest failures on recent pushes main, but I don't see pyright failing at all

@srittau
Copy link
Collaborator Author

srittau commented Nov 25, 2024

@AlexWaygood
Copy link
Member

Huh, that's very odd... Has it been happening on any other PRs that you know of?

@srittau
Copy link
Collaborator Author

srittau commented Nov 25, 2024

I've merged the latest changes for the unnecessary allowlist entries into a few PRs. Let's see what happens.

@AlexWaygood
Copy link
Member

AlexWaygood commented Nov 25, 2024

Okay, it's not really a flake: the pyright: ignore is consistently needed on main but consistently not needed on that PR.

It's because that PR adds stubs that depend on django-stubs, and django-stubs depends on tomli. So that PR pulls tomli into the environment and means that pyright can resolve the import in the setuptools stubs. So the pyright failure in that PR is a side effect of the fact that we have no isolation between stubs packages when we test them with pyright in CI -- all non-types dependencies get installed into one big "venv soup".

@srittau
Copy link
Collaborator Author

srittau commented Nov 25, 2024

Indeed. The question is how to solve it. Maybe we just depend on tomli in requirements for Python < 3.11?

@srittau srittau marked this pull request as draft November 25, 2024 17:16
@AlexWaygood
Copy link
Member

Alternatively... we could maybe just delete this module from the stubs? It seems like an implementation detail at runtime to me; I'm not sure it's meant to be for public consumption. It's probably not worth the bother to maintain stuff like this? @Avasam, what do you think?

Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@Avasam
Copy link
Collaborator

Avasam commented Nov 25, 2024

I concur with Alex' assessment of the CI failure.

And yes setuptools.compat is intended to be private pypa/setuptools#4609 (comment) & https://github.com/jaraco/skeleton/blob/gh-pages/index.md#compatibility-modules
Unless we have a niche case where a type in setuptools.compat is used for typing and the two branches differs in a meaningful way, I think we can remove these modules from typeshed.

@srittau srittau closed this Nov 25, 2024
@srittau srittau deleted the warning-fix branch November 25, 2024 19:00
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