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

Improve YamlError ADT #271

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Improve YamlError ADT #271

wants to merge 1 commit into from

Conversation

sherpal
Copy link

@sherpal sherpal commented Mar 13, 2024

I am opening this pull request to discuss the idea of improving (and "opening") the YamlError ADT.

Currently, only "ascii" messages are provided to the end users. So after calling a method like asNode on a String, all underlying information (position of the error, token responsible, expected token kind) are no more available.

The ParseError is the most obvious enhancement, but the same philosophy can probably be applied to others, like the ComposeError, for example.

This will be a (slight) breaking change (in the case people were destructuring the ParseError case class in a match statement, for example) but since it will only lead to compile-time error, I don't think it's a big deal.

I'm adding a regression test to verify that the actual error message did not change (and will not, at least not unwillingly).

@roman-mibex-2
Copy link

Also having it on the 'ConstructError' would help. When you construct data types from the YAML-Nodes and fail, you want to know where you failed.

Copy link
Member

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

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

LGTM from me.

@lwronski @kpodsiad do you want to take a look?

By providing the `ParseError`, `ConstructError` and `ScannerError` the original information that caused the error, an end user can use it for better diagnostics down the line.
@sherpal
Copy link
Author

sherpal commented Mar 13, 2024

I added the same treatment to ConstructError and ScannerError, though I feel less confident for the design in the ConstructError case.

@lbialy
Copy link

lbialy commented Mar 16, 2024

better question here is: are these all of the changes that users could possibly have for errors? it would be probably good to do this change once since it breaks b/compat

@kpodsiad
Copy link
Contributor

@tgodzik looks fine :)

@sherpal
Copy link
Author

sherpal commented Mar 18, 2024

better question here is: are these all of the changes that users could possibly have for errors? it would be probably good to do this change once since it breaks b/compat

It would be nice, indeed, Do you see any other things were we should/could add more information?

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

6 participants