Skip to content

Fixing signature for Mapping.get's default parameter - #2810

Merged
JelleZijlstra merged 1 commit into
python:masterfrom
mrkmndz:mappingget
Feb 23, 2019
Merged

JelleZijlstra merged 1 commit into
python:masterfrom
mrkmndz:mappingget

Conversation

@mrkmndz

@mrkmndz mrkmndz commented Feb 22, 2019

Copy link
Copy Markdown
Contributor

This better reflects the semantics of the method, (i.e. returning either the value type or the default type), synchronizes it with werkzeug.datastructures.TypeConversionDict and werkzeug.datastructures.Headers, and stops making pyre think that _T is free when default is a _VT_co

@mrkmndz

mrkmndz commented Feb 22, 2019

Copy link
Copy Markdown
Contributor Author

Test failure seems benign (just changed the signature that was being surfaced in a error message). How does one update the mypy tests to match a new typeshed version?

@srittau srittau left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This change seems reasonable to me. I'd like to wait for feedback from the mypy team, though.

@ilevkivskyi

Copy link
Copy Markdown
Member

LGTM. The process for such situations is to merge here despite the failure, then make a PR to mypy, that syncs the typeshed commit and updates the error message. After PR is merged in mypy, things will be green here again. It is better minimize time between merging here and making the mypy PR.

@JelleZijlstra

Copy link
Copy Markdown
Member

Looks good to me too. Could you make a PR to mypy to make the corresponding change there? I'll then be able to merge both.

@mrkmndz

mrkmndz commented Feb 22, 2019

Copy link
Copy Markdown
Contributor Author

How do I sync the typeshed commit in my mypy pr if this one isn't merged yet?

@gvanrossum gvanrossum left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

IIUC @JelleZijlstra will do the merge dance?

@JelleZijlstra
JelleZijlstra merged commit 3f83195 into python:master Feb 23, 2019
@mrkmndz
mrkmndz deleted the mappingget branch February 25, 2019 16:38
@ilevkivskyi

ilevkivskyi commented Feb 27, 2019 •

Copy link
Copy Markdown
Member

It looks like we will need to revert this PR. This causes significant troubles in real code. Consider this example:

m: Mapping[str, List[str]]

x = m.get('x', [])

reveal_type(x)

This used to produce builtins.list[builtins.str] but now it produces Union[builtins.list[builtins.str], builtins.list[<nothing>]] causing subsequent failures because of invariance.

The old signature provided type context for inference of the default argument, and was not wrong in any sense. I will make a PR to revert this now.

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.

5 participants