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

Add error recovery #94

Draft
wants to merge 22 commits into
base: dev
Choose a base branch
from
Draft

Conversation

NebelNidas
Copy link
Collaborator

@NebelNidas NebelNidas commented Dec 14, 2023

Parsers no longer fail when encountering invalid entries. Depending on the situation, the issue is either

  • logged as info when entries are not 100% conformant but data isn't affected in any way,
  • logged as a warning when non-vital mapping data is missing, or
  • logged as an error when elements had to be skipped entirely.

These logs are written to an ErrorSink, which consumers can either implement themselves for custom behavior, or simply use the default implementation via the noOp() factory method. I also added the ErrorCollector interface, which is an extension of ErrorSink, keeping track of all the occurred errors.
To keep compatibility with older versions, by default a ThrowingErrorSink is used, which throws an IOException upon receiving an error of a certain severity, WARNING by default to roughly mirror legacy behavior.

@NebelNidas NebelNidas added this to the 0.7.0 milestone Dec 14, 2023
@@ -208,10 +211,23 @@ public static List<String> getNamespaces(Reader reader, MappingFormat format) th
* @param visitor The receiving visitor.
* @throws IOException If the format can't be detected or reading fails.
*/
@Deprecated
Copy link
Member

Choose a reason for hiding this comment

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

Why deprecate this, I think by default it should fail when it reads a malformed mapping file?

Copy link
Collaborator Author

@NebelNidas NebelNidas Apr 18, 2024

Choose a reason for hiding this comment

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

To not have a dozen different overloads. You can use the ErrorSink#noOp() or ErrorSink#throwingOnSeverity(...) factory methods, just like you'd use ProgressListener.none() in Enigma

Copy link

Choose a reason for hiding this comment

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

Is ther why new ThrowingErrorSink(Severity.WARNING) is a bad default severity? Overloads is like a way to imply default arguments.

Copy link
Collaborator Author

@NebelNidas NebelNidas Apr 18, 2024

Choose a reason for hiding this comment

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

It hinders readability and is a pain to maintain. We already have up to five overloads per mapping reader, imagine we were to add an additional argument in the future, it would double the count again. MappingReader#read already has eight overloads, which I'm pretty sure starts getting confusing for consumers.

At least the latter one would be remedied by #56, which gets rid of some of the overloads by removing implicit format detection.

Copy link

Choose a reason for hiding this comment

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

If we have too many optional arguments, we might just use builders.

add(Severity.ERROR, message);
}

void add(Severity severity, String message) throws IOException;
Copy link
Member

Choose a reason for hiding this comment

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

Could the line number and column be optionally passed though? I see a lot of them include it in the message but might be nice to access this data programmatically?

@NebelNidas NebelNidas removed this from the 0.7.0 milestone Aug 22, 2024
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.

3 participants