Skip to content

requests: allow non-mutable Mapping for files/hooks parameters #7732

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
Apr 28, 2022

Conversation

milanboers
Copy link
Contributor

Another one 😒 still an issue with #7724 . Problem is MutableMapping is invariant, so the union type will actually never match.
Changed it to Mapping (which is covariant in the value type) now. It doesn't seem to be mutated anywhere, I also passed frozendict to try. Also tried assigning a dict[str, BufferedReader] to it as in the issue, which passes the type check now.

Same problem would occur in _Hooks, so changed that one too. Kept _TextMapping as-is because it requires str (would still fail on a subtype of str).

@AlexWaygood
Copy link
Member

AlexWaygood commented Apr 28, 2022

Thanks for all your work on this!

@shawnwall
Copy link

thank you! i tried digging but am not well versed in the ways of how tests work in typeshed, but shouldn't we try to get some test cases in place regarding these types of files= usages to make sure we are all covered in future?

@Akuli
Copy link
Collaborator

Akuli commented Apr 28, 2022

Typeshed doesn't do much tests in the usual sense. There has been a lot of discussion about this, but in short, we don't want to duplicate information that is already specified in the stubs.

For standard library stubs, we have mypy-primer, which runs mypy across many open-source projects on each new PR and posts a diff of mypy outputs as a comment. It would work very well for problems like this. Unfortunately it is currently not being used for third-party stubs, see hauntsaninja/mypy_primer#7.

@JelleZijlstra
Copy link
Member

Though we do have a few assert_type()-based tests now, and we could start adding more.

@brandonchinn178
Copy link

FYI This breaks session.hooks['response'] = ... #7776

@Akuli
Copy link
Collaborator

Akuli commented May 18, 2022

Unfortunately it is currently not being used for third-party stubs

Now it is, and regressions like this are much less likely to happen: hauntsaninja/mypy_primer#31

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.

6 participants