Fix Diagnostic Positions

[trajamsmith]

See issue #260

[coveralls]

Coverage increased (+0.4%) to 77.446% when pulling c45203a on trajamsmith:fix-diagnostic-positions into a9169da on redhat-developer:master.

[trajamsmith]

Moving things around a little in yamlParser07.ts to try and make the logic easier to follow. Adding more typing where possible.

@JPinkney Please let me know if you'd rather I didn't move things around (e.g. extracting helpers into their own util file). I've been running yarn test after every change, but if there's more I need to do to prevent regressions, I'm obviously happy to do it!

[trajamsmith]

Looking at the pipeline, clearly I should be compiling after every step too — on it.

[JPinkney]

Moving around things works for me if it makes the code cleaner 👍 Let me know when this PR is ready to review and i'll take a look!

[trajamsmith]

So, as I dig deeper on this, the issue is that the exceptions from yaml-ast-parser (or the fork we're using here) are not very useful for constructing the actual language server diagnostics.

While the exceptions give accurate starting positions (including line and column numbers), there is — as far as I can tell — no good way to reconstruct the exceptions' ending positions. I've tried doing so with the getSnippet() method from the library, but it does some weird, unhelpful formatting that I'm hesitant to manually undo.

I'm not really sure what the next step is. I'm now getting accurate diagnostics for a few error types, and only a couple of tests are failing right now, but I'm really not confident that trying to hack around the AST parsing library won't just create more problems not reflected in the tests.

I could, of course, be completely overlooking something obvious, but it seems like we'd need to build out a different approach for the diagnostic generation. I'll investigate this further, but diagnostic positions way back in 0.4.0 seemed to be doing fine. I think that was before switching to the yaml-ast-parser (could be wrong).

[JPinkney]

We've had the yaml ast parser since the beginning and yaml-ast-parser-custom-tags (the one we use) should just be master of yaml-ast-parser with one patch ontop. If I were to guess, the problems arose with this massive PR: https://github.com/redhat-developer/yaml-language-server/pull/147/files. I'll have to take a deeper look.

One noticeable thing I've found is that convert error in 0.4.1 seems to use a much simpler convertError function: https://github.com/redhat-developer/yaml-language-server/blob/0.4.1/src/languageservice/parser/yamlParser.ts#L203. I can't recall why the convertError function was changed and I can't find any associated issue with that change either. So maybe the solution is to just change the convertError function back

[JPinkney]

It looks like doing something like:

const line = e.mark.line < 0 ? 0 : e.mark.line;
const character = e.mark.position + e.mark.column < 0 ? 0 : e.mark.position + e.mark.column;

in convert error might work. The only issue with that code is it seems to be showing the error on the value instead of the key.

const line = e.mark.line < 0 ? 0 : e.mark.line;
const character = e.mark.column + 1;

might work as well. It seems to show the error on the correct location in my limited testing

[trajamsmith]

Sorry for being a little slow with this PR — @JPinkney if you want to take just a quick peek at this, I'm pretty sure that I successfully reverted the diagnostic positions to how they were handled in 0.4.1. Let me know if anything looks awry.

[trajamsmith]

I feel pretty confident that the old test was wrong — the error should be on character two, the line doesn't extend to a thirteenth character.

This is in line with the logging I did on the testing suite in 0.4.1.

[trajamsmith]

[trajamsmith]

^output from 0.4.1

[trajamsmith]

And just to document this here:
@JPinkney You were totally right that the diagnostics were using the yaml-ast-parser without any additional shenanigans.

I didn't realize that the language server spec includes a function that will determine the line and character position from the absolute position in the string, TextDocument.positionAt(). Somehow in that big update after 0.4.x, the line and character positions were no longer being calculated using that method.

[JPinkney]

Super small nitpicks but overall it's really good! I'm excited to get this in

[JPinkney]

Oops meant to click request changes, just for the nitpicks then I'll merge 😉

[JPinkney]

Also it will need to be rebased

[trajamsmith]

Implemented the requested changes — let me know if we need anything else for a merge!

I think the positions are still a little weird with some kinds of errors, but reverting to the old working implementation I think will fix a lot of them.