Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

End position in LSP range is exclusive #2223

Merged
merged 2 commits into from
Jan 21, 2019

Conversation

andreypopp
Copy link
Contributor

From LSP spec:

A range in a text document expressed as (zero-based) start and end
positions. A range is comparable to a selection in an editor. Therefore
the end position is exclusive.

https://microsoft.github.io/language-server-protocol/specification

Where are the tests? Have you added tests? Have you updated the tests? Read the
comment above and the documentation referenced in it first. Write tests!

Seriously, read :help ale-dev and write tests.

From LSP spec:

> A range in a text document expressed as (zero-based) start and end
> positions. A range is comparable to a selection in an editor. Therefore
> the end position is exclusive.
@andreypopp
Copy link
Contributor Author

Will update the tests if you agree with this change.

@w0rp
Copy link
Member

w0rp commented Jan 21, 2019

I think this might be right. It's hard to tell. Can you share some examples of highlights or something being corrected by this change? Some screenshots with example language servers would be helpful.

@andreypopp
Copy link
Contributor Author

andreypopp commented Jan 21, 2019

This is what I see with ocamlmerlin-lsp:
screenshot 2019-01-21 at 17 58 33

This is from @TrySound with flow lsp:
unnamed

As you can see the ( shouldn't be highlighted. I also crossvalidated these with VSCode and it treats same responses correctly.

@andreypopp
Copy link
Contributor Author

Can't find what's wrong with appveyor by looking at logs... it seem to repo everything is ok...

@andreypopp
Copy link
Contributor Author

Found the failed tests but not sure why they are failing:

[00:01:38]   Starting Vader: C:\testplugin\test\smoke_test.vader
[00:01:38]     (1/3) [  GIVEN] Some imaginary filetype
[00:01:38]     (1/3) [EXECUTE] Linters should run with the default options
[00:01:38]     (2/3) [  GIVEN] Some imaginary filetype
[00:01:38]     (2/3) [EXECUTE] Linters should run in PowerShell too
[00:01:38]     (2/3) [EXECUTE] (X) [{'lnum': 4, 'bufnr': 3, 'col': 3, 'valid': 1, 'vcol': 0, 'nr': -1, 'type': 'E', 'pattern': '', 'text': 'foo bar'}] should be equal to [{'lnum': 1, 'bufnr': 3, 'vcol': 0, 'pattern': '', 'valid': 1, 'nr': -1, 'type': 'E', 'col': 3, 'text': 'foo'}, {'lnum': 2, 'bufnr': 3, 'vcol': 0, 'pattern': '', 'valid': 1, 'nr': -1, 'type': 'E', 'col': 3, 'text': 'bar'}]
[00:01:38]     (3/3) [  GIVEN] Some imaginary filetype
[00:01:38]     (3/3) [EXECUTE] Previous errors should be removed when linters change
[00:01:38]     (3/3) [EXECUTE] (X) Vim(return):E684: list index out of range: 0
[00:01:38]       > function ale#engine#WaitForJobs[51]..<SNR>37_VimCloseCallback[14]..<SNR>34_HandleExit[44]..TestCallback, line 2
[00:01:38]   Success/Total: 1/3

Seem unrelated to this PR.

@w0rp
Copy link
Member

w0rp commented Jan 21, 2019

That's probably just a test that fails randomly some times.

@w0rp w0rp merged commit 37107df into dense-analysis:master Jan 21, 2019
@w0rp
Copy link
Member

w0rp commented Jan 21, 2019

Cheers! 🍻

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.

None yet

2 participants