Fix the end range of the diagnostics. #567
Conversation
| Position start = new Position(); | ||
| Position end = new Position(); | ||
|
|
||
| start.setLine(problem.getSourceLineNumber() - 1);// VSCode is 0-based |
There was a problem hiding this comment.
jdt.ls is client agnostic, so we should rather say "clients are expected to be 0-based", or something like that
There was a problem hiding this comment.
Unless I'm missing something, the protocol is 0-based
A range in a text document expressed as (zero-based) start and end positions.
Clients are usually 1-based (users count from 1) :)
| private static Range convertRange(IOpenable openable, IProblem problem) { | ||
| try { | ||
| return JDTUtils.toRange(openable, problem.getSourceStart(), problem.getSourceEnd() - problem.getSourceStart() + 1); | ||
| } catch (CoreException e) { |
There was a problem hiding this comment.
in which case are we expected to catch CoreExceptions?
There was a problem hiding this comment.
When try to access the Buffer of the IOpenable obect, it will throw JavaModelException. In this case, just use the previous behavior, which only based on the information of IProblem.
There was a problem hiding this comment.
ok please add a comment in the code then
|
PR looks to be working fine, this is a welcome improvement @yaohaizh! |
|
So we need to apply #558 before so dead code generates multiline problems? |
|
Right, we need fix this one for dead code on VSCode. Otherwise, the VSCode cannot detect the code actions correctly. But other client might have different behavior for multiple-line IProblem. |
yaohaizh
force-pushed
the
yaohai_diagnostic
branch
from
1b99aa9
to
383cc9d
Compare
| @@ -117,15 +117,28 @@ public void testUnimplementedMethods() throws Exception { | |||
| ICompilationUnit cu = pack1.createCompilationUnit("F.java", buf.toString(), false, null); | |||
| openDocument(cu, cu.getSource(), 1); | |||
|
|
|||
| buf = new StringBuilder(); | |||
| List<Command> commands = getCodeActions(cu); | |||
| assertTrue(commands.size() == 1); | |||
There was a problem hiding this comment.
never ever use assertTrue when assertEquals can do the job, you lose important diagnostic informations, i.e what was the actual value received.
| buf.append(" }\n"); | ||
| buf.append("}\n"); | ||
| ICompilationUnit cu = pack1.createCompilationUnit("E.java", buf.toString(), false, null); | ||
|
|
||
| List<Command> commands = getCodeActions(cu); | ||
| assertTrue(commands.size() == 1); |
| } | ||
|
|
||
| @Test | ||
| public void testRemoveDeadCodeAfterIf() throws Exception { |
There was a problem hiding this comment.
this test doesn't check for the range is multi-line, i.e what we're trying to fix in the 1st place
yaohaizh
force-pushed
the
yaohai_diagnostic
branch
from
383cc9d
to
00bebde
Compare
|
Updated the PR. |
Fix the issue mentioned here: microsoft/vscode#43878
The end range is always be the same line as the start, which should not be the correct behavior. As this issue will cause multiple lines problems cannot be detected in the VSCode.
@fbricon @gorkem