KEMBAR78
handle Union type in string interpolation checker #7763 by vbarbaresi · Pull Request #7809 · python/mypy · GitHub
Skip to content

Conversation

vbarbaresi
Copy link
Contributor

The problematic case was

t: Union[Tuple[str, int], Tuple[int, str]] = ('A', 1)
"%s %s" % t

The Union was treated as a single value instead of looking at the types inside
The fix consists in checking each of the union types

Two questions:

  • I created a TempNode for each of the sub types and added the current line to keep some context, not sure it's the right way to do it.
  • I'm running the check for all of the types in the union, but I've seen in the code base that it's usually checked for any().
    I suppose it shouldn't return an error if one of the types matches the formatting arguments, for instance with Union[Tuple[str, int], Tuple[str, str, str]]
    If you confirm, I will have to refactor part of the check in a sub-method

The problematic case was

```
t: Union[Tuple[str, int], Tuple[int, str]] = ('A', 1)
"%s %s" % t
```

The Union was treated as a single value instead of looking at the types inside

The fix consists in checking each of the union types
Copy link
Collaborator

@JukkaL JukkaL 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 the PR! Looks good.

Answers to questions:

  • Using TempNode is a reasonable approach.
  • Operations on union types must be valid for all union items.


b: Union[Tuple[int, str], Tuple[int, int], Tuple[str, int]] = ('A', 1)
'%s %s' % b
'%s %s %s' % b # E: Not enough arguments for format string
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add test case where one union item is valid and another is invalid. It should be an error if any item is invalid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done ✅

Copy link
Collaborator

@JukkaL JukkaL 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 the quick response!

@JukkaL JukkaL merged commit f0df1d6 into python:master Oct 29, 2019
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.

2 participants