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 list concatenation #2404

Merged
merged 5 commits into from
Nov 8, 2018
Merged

fix list concatenation #2404

merged 5 commits into from
Nov 8, 2018

Conversation

JelleZijlstra
Copy link
Member

Fixes #2383, python/mypy#5492.

I was a little surprised this works, but it does. I tried the test cases from both of the linked issues, and mypy allows them without any errors when this typeshed patch is applied.

Unfortunately, with this change __iadd__ becomes incompatible with __add__ according to mypy. I tried changing it to def __iadd__(self: List[_T_co], x: Iterable[_T_co]) -> List[_T_co]: ..., which passes mypy, but that breaks this test case:

from typing import List
x: List[object]
y: List[int]
x += y

Therefore, I chose to # type: ignore it instead.

@ilevkivskyi
Copy link
Member

I am not sure if this is the right solution. Maybe we can try something closer to set.__or__/set.__ior__? Will this fix the issues?

I guess a perfect solution for this kind of questions would require python/mypy#2354 solved first.

@JukkaL
Copy link
Contributor

JukkaL commented Oct 12, 2018

The return type needs to use a union, similar to the set operations.

I don't think that generic self types would help, since subclasses will default to generating list instances:

class A(list): pass
type(A([1]) + A([1])) is list  # True

@ilevkivskyi
Copy link
Member

I don't think that generic self types would help, since subclasses will default to generating list instances

Yes, indeed, it returns just a list. I remember people wanted generic self types when they want the return to be a subtype, but it is not the case here.

The return type needs to use a union, similar to the set operations.

This actually might not solve the original issue, where a join is preferred:

class B: ...
class C1(B): ...
class C2(B): ...

lst: List[B] = [C1()] + [C2()]

This is however an eternal question of union vs join, so I think there is no ideal solution here.

@ilevkivskyi
Copy link
Member

Just to clarify I do think returning a List[Union[T, S]] is a better compromise.

@JelleZijlstra
Copy link
Member Author

I am going to be out for another week and won't have time to update the code in this PR. If it's urgent, feel free to take over the PR and change the code as you see fit; otherwise I'll implement your suggestion in a few weeks.

@JelleZijlstra
Copy link
Member Author

I implemented the Union return type suggestion now.

Copy link
Member

@ilevkivskyi ilevkivskyi left a comment

Choose a reason for hiding this comment

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

LGTM!

@ilevkivskyi ilevkivskyi merged commit 1a42a2c into python:master Nov 8, 2018
@JelleZijlstra JelleZijlstra deleted the listadd branch November 8, 2018 17:18
JukkaL added a commit to JukkaL/typeshed that referenced this pull request Nov 29, 2018
The fix caused regressions for mypy that are difficult to
fix.  See python/mypy#5492 for
context.

This reverts commit 1a42a2c.
JelleZijlstra pushed a commit that referenced this pull request Nov 29, 2018
The fix caused regressions for mypy that are difficult to
fix.  See python/mypy#5492 for
context.

This reverts commit 1a42a2c.
yedpodtrzitko pushed a commit to yedpodtrzitko/typeshed that referenced this pull request Jan 23, 2019
yedpodtrzitko pushed a commit to yedpodtrzitko/typeshed that referenced this pull request Jan 23, 2019
The fix caused regressions for mypy that are difficult to
fix.  See python/mypy#5492 for
context.

This reverts commit 1a42a2c.
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