Add error end functionality #62
Conversation
|
According to LSP docs the end position is exclusive so I just fixed that with the second commit |
pylsp_mypy/plugin.py
Outdated
| errno = 2 | ||
| if severity == "error": | ||
| errno = 1 | ||
| diag: Dict[str, Any] = { | ||
| "source": "mypy", | ||
| "range": { | ||
| "start": {"line": lineno, "character": offset}, | ||
| # There may be a better solution, but mypy does not provide end | ||
| "end": {"line": lineno, "character": offset + 1}, | ||
| "end": {"line": end_lineno or lineno, "character": end_offset}, |
There was a problem hiding this comment.
Those can be None and None is not allowed in Position.
I see that you later adjust character if it's None but not line.
You should probably fall back to previous behavior in that case.
There was a problem hiding this comment.
I've been trying to fix the failing test for 3.7 but it only seems to make other tests fail and the code is really unreadable. The main reason the test is failing right now is because running mypy in 3.7 with --show-error-end sets the end position the same as start position. I think the best thing to do would be to detect this case and add the word lenght to end position if that is the case. This would require rewriting the test_plugin test case.
As for your comment. I think the best solution would be to remove the posibility for positions (and other fields) to be optional. Even in 3.7 they will always print (because --show-error-end is hardcoded and can't be disabled) so there is no point writing regex that allows for them to be missing. This would greatly simplify the code and we can also remove some tests that check those positions missing. I might be missing something though.
There was a problem hiding this comment.
So mypy with this option behaves differently in 3.7? (Tests seem to indicate that, I just want to confirm)
Is this documented in mypy as intended or is this a bug?
Either way we will not write a lengthy workaround as 3.7 support will likely be dropped soon anyway.
There was a problem hiding this comment.
Didn't find anything in docs or an issue but I did try with multiple different error types and they all had the same behaviour.
Can you clarify what you mean by not supporting a lengthy workaround.
There was a problem hiding this comment.
Maybe we should ask the mypy devs if this is intentional or a bug.
Mypy seems to behave differently in python 3.7 than in the other versions. In order to handle this, 'unreadable code' (as you put it) is required. This is what I mean by 'lengthy workaround'. Having this in the codebase is not desirable in the first place, but in this case there is another point to consider. Python 3.7 will be end of life very soon. Mypy will most likely drop support for it then and so will we. This means, that in approx two months our code does not need to work with python 3.7 anymore. So there is little reason to try really hard to handle some special case in 3.7 now.
So: If the test for 3.7 does not work, just ignore it for the time being.
Cleaning up the regex is fine with me and probably a good idea.
- Change pattern so that it only accepts diagnostics with full details - Rewrite `parse_line` - Remove redundant tests
|
Sorry for the wait but I fixed it in what I think is the best way possible and I also manually ran tests in all 5 versions and all seems to work fine. It is a lot of changes though. In short I removed the redundant tests that were testing for lines with partial information. I also modified the other tests so that they check that other fields of diagnostic are also correctly parsed. |
|
Don't worry. I appreciate every contribution. How did you handle the python 3.7 I don't see any special code for that? |
|
As you said I didn't. I only modified the test case that depended on the actual output of mypy so taht it would pass in 3.7. The other tests use predefined output lines so they were fine. |
|
Fyi, I made a bug report at python/mypy#15281 to assure 3.7 is actually broken, and we did not misunderstand something. |
Code change for issue #61.
I also noticed that the field
codeofDiagnositcis unused but rather errors were parsed in such a way that the mypy error code was included in the diagnostic message. I think this is a bug and am also willing to fix it.