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

Use JSON output for ESLint and fix tsserver column #2551

Merged
merged 2 commits into from
Jun 8, 2019

Conversation

laino
Copy link
Contributor

@laino laino commented Jun 2, 2019

This makes ale use ESLint's JSON formatter/reporter instead of the default line-based one. Besides being cleaner, this has the added benefit of giving us native end_col values:

image
Old vs. New

I have kept the old parser around, because the standard linter does not support JSON output yet, and uses the ESLint output parser internally. The xo linter has been migrated to the JSON parser though.

The new handler is at ale#handlers#eslint#HandleJSON, while the old line-based parser is still at ale#handlers#eslint#Handle.

I have added tests 1:1 as they existed for the old parser, except this time for JSON output of course.

There was a test that dealt with something that is only relevant for the old parser, which is for ESLint adding "Error/" in front of some error codes. ESLint doesn't do this with JSON output, so that test has no equivalent.

Additionally tsserver appears to report [a, b[ ranges instead of [a,b], so I fixed that off-by-one error as well. This is also visible in the screenshot above (red error).

ale_linters/typescript/xo.vim Show resolved Hide resolved
@@ -90,7 +90,7 @@ function! ale#lsp#response#ReadTSServerDiagnostics(response) abort
\ 'lnum': l:diagnostic.start.line,
\ 'col': l:diagnostic.start.offset,
\ 'end_lnum': l:diagnostic.end.line,
\ 'end_col': l:diagnostic.end.offset,
\ 'end_col': l:diagnostic.end.offset - 1,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this is right. I'll check it with some errors in my codebase.

@laino
Copy link
Contributor Author

laino commented Jun 7, 2019

@w0rp Have you been able to confirm the correctness of the tsserver fix on your end?

@w0rp w0rp merged commit 5826b49 into dense-analysis:master Jun 8, 2019
@w0rp
Copy link
Member

w0rp commented Jun 8, 2019

I checked it now and it works. 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