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

[WIP] Fix issue #4975 #8253

Closed
wants to merge 3 commits into from
Closed

Conversation

theodoretliu
Copy link
Contributor

Addresses issue #4975. See three test cases, first is passing, unclear on what the behavior should be on second two

@theodoretliu
Copy link
Contributor Author

@ilevkivskyi @Michael0x2a appears to be working if I hardcode a file test.py and run mypy on it

from typing import Tuple

x: Tuple[str, float]
y: Tuple[int, ...]

lst = [x, y]
reveal_type(lst[0])

produces

test.py:7: note: Revealed type is 'builtins.tuple*[builtins.object]'

with no errors.

However, running the same in the test suite seems to cause issues.

Similar with the following

from typing import Tuple

x: Tuple[int, str]
y: Tuple[int, ...]

lst = [x, y]
reveal_type(lst[0])

produces

test.py:7: note: Revealed type is 'builtins.tuple*[builtins.object]'

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.

Thanks for PR!

The tests failed because the list.pyi test fixture defines tuple as invariant, you can update it. Alternatively you can use tuple.pyi fixture.

I don't think this covers all the cases in the original issue and the issues that were closed as duplicates. In particular, these test cases are missing:

  • Join two tuples of different length: Tuple[str, str] vs Tuple[str, str, str]
  • Ditto with different types
  • Joins with an empty tuple (fixed length and variable length)
  • It would be great to have a test with ternary expressions instead of lists.

You can browse through linked issues, maybe there are more.

mypy/join.py Outdated Show resolved Hide resolved
@ilevkivskyi ilevkivskyi removed their request for review January 13, 2020 14:36
@ilevkivskyi
Copy link
Member

Sorry, I will not have time for this, hopefully @JukkaL or @msullivan can help.

@JukkaL
Copy link
Collaborator

JukkaL commented Jan 14, 2020

I can finish the review. The next steps would be fixing the merge conflict and the failing test cases. If you need help with some of these, just let me know.

@intgr
Copy link
Contributor

intgr commented Jan 26, 2020

Doh, sorry :( I was under the impression that nobody was working on this issue (made that assumption when JelleZijlstra commented in #4975). It didn't help that this PR is titled "Initial attempt"

So meanwhile I submitted two pull requests for the two behaviors implemented here: #8333 and #8335.

The good news is that I happened to find a cleaner implementation, my approach can join Tuple[x, x] + List[x] into a Sequence[x] type, which seems to not be the case here.

I now tested my code with the test cases in this PR and my code passes these cases.

What do you think is the best way forward?

@theodoretliu
Copy link
Contributor Author

@intgr well, you were quick on the uptake, so I don't have a problem if they merge yours instead of mine. In the future, I can title my PR better, but I did reference the issue in this PR and it does show up on the Issue tracker... Nevertheless, taking a quick look at your PR shows that we had approximately the same idea, it's up the maintainers @JukkaL @ilevkivskyi to make the final call.

There was an issue with my PR that was raised regarding types that inherit from Tuple: if we go with yours, we might try to fix that too?

@theodoretliu theodoretliu changed the title Initial attempt [WIP] Fix issue #4975 Jan 26, 2020
@intgr
Copy link
Contributor

intgr commented Jan 26, 2020

Thanks, for the heads-up, I hadn't considered Tuple subclasses. I'll have to test that behavior.

@JukkaL
Copy link
Collaborator

JukkaL commented Jan 29, 2020

Hmm, I think I prefer #8333 and #8335, since they are more recent and have no conflicts. Supporting tuple subclasses would be a nice addition.

@theodoretliu
Copy link
Contributor Author

Closing in favor of @intgr work in #8333 and #8335

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.

4 participants