Make str unpacking check opt-in #15511
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes python#13823. See also python#6406
for more information, see https://pre-commit.ci
|
Diff from mypy_primer, showing the effect of this PR on open source code: pandas-stubs (https://github.com/pandas-dev/pandas-stubs)
- tests/test_frame.py:2001: error: Unpacking a string is disallowed [misc]
- tests/test_frame.py:2007: error: Unpacking a string is disallowed [misc]
- tests/test_series.py:606: error: Unpacking a string is disallowed [misc]
- tests/test_series.py:612: error: Unpacking a string is disallowed [misc]
operator (https://github.com/canonical/operator)
- ops/framework.py:166: error: Unpacking a string is disallowed [misc]
aiohttp (https://github.com/aio-libs/aiohttp)
+ aiohttp/client_reqrep.py:350: error: Unused "type: ignore" comment [unused-ignore]
+ aiohttp/web_log.py:206: error: Unused "type: ignore" comment [unused-ignore]
+ aiohttp/web_log.py:207: error: Unused "type: ignore[has-type]" comment [unused-ignore]
+ aiohttp/web_log.py:208: error: Unused "type: ignore[has-type]" comment [unused-ignore]
+ aiohttp/web_log.py:209: error: Unused "type: ignore[has-type]" comment [unused-ignore]
|
JukkaL
approved these changes
Jun 26, 2023
|
|
||
| .. _code-str-unpacking: | ||
|
|
||
| Check that ``str`` is explicitly unpacked [str-unpacking] |
There was a problem hiding this comment.
The title is a bit unclear. What about something like "Check that 'str' is not unpacked?". Or "... is not unpacked implicitly", though I'm not sure what's the difference between explicit and implicit unpacking here.
Bikeshedding: what about renaming the error code to str-unpack, which would be a bit shorter?
| Check that ``str`` is explicitly unpacked [str-unpacking] | ||
| --------------------------------------------------------- | ||
|
|
||
| It can sometimes be surprising that ``str`` is iterable, especially when unpacking. |
There was a problem hiding this comment.
Explicitly enumerate the contexts where this matters (for loop, assignment)?
| @@ -3491,7 +3491,11 @@ def check_multi_assignment( | |||
| self.check_multi_assignment_from_union( | |||
| lvalues, rvalue, rvalue_type, context, infer_lvalue_type | |||
| ) | |||
| elif isinstance(rvalue_type, Instance) and rvalue_type.type.fullname == "builtins.str": | |||
| elif ( | |||
| self.msg.errors.is_error_code_enabled(codes.STR_UNPACKING) | |||
There was a problem hiding this comment.
Is this included in --strict, and should it be?
emmatyping
reviewed
Jun 26, 2023
| elif ( | ||
| self.msg.errors.is_error_code_enabled(codes.STR_UNPACKING) | ||
| and isinstance(rvalue_type, Instance) | ||
| and rvalue_type.type.fullname == "builtins.str" |
There was a problem hiding this comment.
Will this check unpacking on Literal strings? Can you add a testcase for that please?
3 tasks
|
Any idea when this could be merged? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #13823. See also #6406