Add subclasses for transformation result #36
Conversation
|
|
||
| group 'uk.gov.hmcts.reform' | ||
| version '0.0.1' | ||
| version '0.1.0' |
There was a problem hiding this comment.
breaking change (class became abstract)
...va/uk/gov/hmcts/reform/bulkscanccdeventhandler/transformer/model/OkTransformationResult.java
Outdated
Show resolved
Hide resolved
...in/java/uk/gov/hmcts/reform/bulkscanccdeventhandler/handler/ExceptionRecordEventHandler.java
Outdated
Show resolved
Hide resolved
|
|
||
| boolean shouldCreateCase = result.errors.isEmpty() && (result.warnings.isEmpty() || req.ignoreWarnings); | ||
| return handleTransformationResult( | ||
| transformer.transform(req.exceptionRecord), |
There was a problem hiding this comment.
This argument could also be a function. We should handle exceptions thrown by it, perhaps by wrapping them in some tranformation-specific exception.
There was a problem hiding this comment.
Not sure, it's the code provided by the users of the library, it can just throw exception from their code directly. Also no need for a function here imho, it's always called.
The shorter the stacktrace the better probably.
Let's focus on the new 2 types of results on this PR.
There was a problem hiding this comment.
Sure, exception handling and logging can be done separately. The reason why I wanted function was to catch its exceptions. Those could also be caught at this level, doesn't matter that much.
I think it's clearer when the library throws an exception saying that custom transformation code threw an exception, rather than NullPointerException or whatever happened. It increases the likelihood of product teams realising that the problem occurred in their transformer and wasn't a bug in the library.
There was a problem hiding this comment.
Good point. Let's discuss separately.
| boolean shouldCreateCase = result.errors.isEmpty() && (result.warnings.isEmpty() || req.ignoreWarnings); | ||
| return handleTransformationResult( | ||
| transformer.transform(req.exceptionRecord), | ||
| okRes -> { |
There was a problem hiding this comment.
This is minor and can stay as it is, but my suggestion is to change okRest and errRes to full names.
...in/java/uk/gov/hmcts/reform/bulkscanccdeventhandler/handler/ExceptionRecordEventHandler.java
Show resolved
Hide resolved
|
|
||
| import java.util.List; | ||
|
|
||
| public class OkTransformationResult implements TransformationResult { |
There was a problem hiding this comment.
I wonder if we should go with Ok... or Success.... Not very important, anyway.
rkondratowicz
deleted the
feature/add-subclasses-for-transformation-result
branch
2 participants
Should simplify using the library a bit, just provide the data relevant to the 'type' of result.