feat(config): better error messages around schema validation

[edvald]

This attaches the YAML parse results to configs when loading, and leverages that when rendering validation error messages.

...
3  | spec:
4  |   foo: 123
-------^
"foo" should be a string

This should be a useful step for an IDE highlighting integration as well.

[vvagaytsev]

There are some test failures, please take a look.

[TimBeyer]

Does this work as expected with YAML files that contain multiple docs?
When I was trying this out you needed to do something like

const lineCounter = new LineCounter()
const parser = new Parser(lineCounter.addNewLine)

const tokens = parser.parse(content)
const docsGenerator = new Composer().compose(tokens)

const docs = Array.from(docsGenerator) as Document.Parsed<ParsedNode>[]

to get the docs and then use lineCounter.linePos to get the correct line in the input file.

[edvald]

I hadn't tested that specifically, good catch. Not sure I fully follow your code there though. Happy to take commits on this PR btw, it's very much a draft still, bunch of broken tests etc.

[thsig]

Btw, in case it simplifies the implementation I think it's acceptable if we just show the YAML doc/subdoc that the error came from, even it it's a sub-doc of a larger one—we just need to help the user quickly pinpoint the source location.

Tests are now passing

[vvagaytsev]

Does this statement always break the loop after the first iteration? Was it done intentionally? It looks like the returned boolean value is never used, do we need it?

[edvald]

That's intentional. The boolean needs to be there, linter complains if we only return it to break.

[vvagaytsev]

Ok, got it, thanks! It may make sense to leave a short comment on that :)

[vvagaytsev]

I left a few comments and questions, please check.
Otherwise looks good, thank you! 👍

[vvagaytsev]

LGTM! 👍 Thanks for the great improvement! 💯

[vvagaytsev]

@edvald would you have time to resolve the conflicts soon? I can also take a look, please let me know if you need to hand this over. It would be nice to include it in one of the upcoming releases.

[edvald]

Just did that actually, good timing :) Just grappling with my merge commit now.

[edvald]

Ok, should be ready now, pending tests passing.

[vvagaytsev]

Now there are more conflicts to be resolved.

[edvald]

Updated again.

[vvagaytsev]

It looks like these parameters are never rendered. Do we need this specific kind of error? Or should we just use ConfigurationError class instead?
@stefreak @TimBeyer wdyt?

[edvald]

I opted to keep this even though the typed detail object was removed, since we are likely to use this (and potentially more) later for e.g. a language server implementation.

[vvagaytsev]

Thank you! 💯