Add error recovery #94
Conversation
src/main/java/net/fabricmc/mappingio/format/ErrorCollector.java
Outdated
Show resolved
Hide resolved
src/main/java/net/fabricmc/mappingio/format/ErrorCollector.java
Outdated
Show resolved
Hide resolved
src/main/java/net/fabricmc/mappingio/format/ErrorCollector.java
Outdated
Show resolved
Hide resolved
Achieved in part by not always seeking to the start of the next column
NebelNidas
force-pushed
the
error-recovery
branch
from
9333443 to
4363dc9
Compare
| @@ -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 | |||
There was a problem hiding this comment.
Why deprecate this, I think by default it should fail when it reads a malformed mapping file?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Is ther why new ThrowingErrorSink(Severity.WARNING) is a bad default severity? Overloads is like a way to imply default arguments.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
If we have too many optional arguments, we might just use builders.
| add(Severity.ERROR, message); | ||
| } | ||
|
|
||
| void add(Severity severity, String message) throws IOException; |
There was a problem hiding this comment.
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?
Parsers no longer fail when encountering invalid entries. Depending on the situation, the issue is either
These logs are written to an
ErrorSink, which consumers can either implement themselves for custom behavior, or simply use the default implementation via thenoOp()factory method. I also added theErrorCollectorinterface, which is an extension ofErrorSink, keeping track of all the occurred errors.To keep compatibility with older versions, by default a
ThrowingErrorSinkis used, which throws anIOExceptionupon receiving an error of a certain severity,WARNINGby default to roughly mirror legacy behavior.