Update mypy error messages in code examples, part 1 #7706
Conversation
Signed-off-by: Oleg Höfling <oleg.hoefling@gmail.com>
Signed-off-by: Oleg Höfling <oleg.hoefling@gmail.com>
Signed-off-by: Oleg Höfling <oleg.hoefling@gmail.com>
Signed-off-by: Oleg Höfling <oleg.hoefling@gmail.com>
Signed-off-by: Oleg Höfling <oleg.hoefling@gmail.com>
| new_lst = [B(), B()] # inferred type is List[B] | ||
| lst = new_lst # mypy will complain about this, because List is invariant | ||
| # mypy will complain about this, because List is invariant | ||
| lst = new_lst # error: Incompatible types in assignment (expression has type "List[B]", variable has type "List[A]") |
There was a problem hiding this comment.
Please advise whether keeping the explanation comments is useful, and whether the selected format is acceptable:
<code> # original explanation becomes
# original explanation
<code> # mypy error
There was a problem hiding this comment.
This looks fine to me. Though here the explanation is pretty informal, and it may be better to be more explicit about what this refers to. Here is one idea:
# Mypy rejects the following assignment, because List is invariant
lst = new_lst # error: ...
| # to silence complaints about unused imports | ||
| from typing import List # noqa | ||
| a = None # type: List[int] | ||
| a = [] # type: List[int] |
There was a problem hiding this comment.
In the original example, mypy reports error: Incompatible types in assignment (expression has type "None", variable has type "List[int]").
|
I'm not actually 100% sure that this is always an improvement. IIRC the messages shown in the doc examples aren't meant (in most cases) to represent exactly what mypy will say, but are an indication of what's wrong with the line in terms that make sense to the narrative. So I'll leave it up to @JukkaL to review this. |
JukkaL
left a comment
JukkaL
left a comment
There was a problem hiding this comment.
I think that it's a good idea to make code examples have more realistic error messages, but if the message is very long, it's better to shorten it for clarity and to avoid long lines that may be cause scrolling.
| test = Application("Testing...") # OK | ||
| bad = Application("Testing...", "with plugin") # Error: List[str] expected | ||
| bad = Application("Testing...", "with plugin") # error: Argument 2 to "Application" has incompatible type "str"; expected "List[str]" |
There was a problem hiding this comment.
I'd try to keep lines a bit shorter to avoid horizontal scrolling in most cases. Maybe keep them under 100 characters? I think that it's fine to shorten messages. For example leaving out to "Application" above seems reasonable. Also, it would be fine to split he comment into multiple lines. Maybe like this:
bad = Application("Testing...", "with plugin") # error: Argument 2 has incompatible type "str";
# expected "List[str]"
| OrderedPoint(1, 2) < OrderedPoint(3, 4) # OK | ||
| UnorderedPoint(1, 2) < UnorderedPoint(3, 4) # Error: Unsupported operand types | ||
| UnorderedPoint(1, 2) < UnorderedPoint(3, 4) # error: Unsupported left operand type for < ("UnorderedPoint") |
There was a problem hiding this comment.
Again, this is a bit long. Maybe leave out ("UnorderedPoint")?
| new_lst = [B(), B()] # inferred type is List[B] | ||
| lst = new_lst # mypy will complain about this, because List is invariant | ||
| # mypy will complain about this, because List is invariant | ||
| lst = new_lst # error: Incompatible types in assignment (expression has type "List[B]", variable has type "List[A]") |
There was a problem hiding this comment.
This looks fine to me. Though here the explanation is pretty informal, and it may be better to be more explicit about what this refers to. Here is one idea:
# Mypy rejects the following assignment, because List is invariant
lst = new_lst # error: ...
|
After some evaluating the remaining docs, I'm not sure I can tackle this - I'd agree with @gvanrossum that not all spots deserve to have the formal error description. Let me close this for now. |
3 participants
First part of changes for #2393.