Improve logging and fix CI#21
Improve logging and fix CI#21jebeaudet merged 6 commits intomaincoveooss/feign-error-decoder:mainfrom fix/improve-loggingcoveooss/feign-error-decoder:fix/improve-loggingCopy head branch name to clipboard
Conversation
| logger.debug("Unable to directly set the detailMessage, make sure exception do implement {}", baseExceptionClass.getName()); | ||
| logger.debug( | ||
| "Unable to set the detailMessage via reflection, make sure the base exception do implement '{}'. Error message: '{}'.", | ||
| ExceptionMessageSetter.class.getName(), |
There was a problem hiding this comment.
It's kind of weird that the class takes anything that extends exception (so is meant to be super flexible?). But will displays the ExceptionMessageSetter class name here. We could probably make it less flexible or respect the baseExceptionClass to avoid confusion.
As I think of it, it's a bit weird that ExceptionMessageSetter is kind of the implicit default ? Shouldn't the constructor use ExceptionMessageSetter as the base class if none is provided? (I think would also allow us to simplify some methods)
There was a problem hiding this comment.
The class cannot restrict the base exception as it parses exceptions from an interface Class<?> apiClass which cannot be typed and the exception declared on the methods are also out of the control of this class.
The constructor already takes a base exception that is typed (S).
ExceptionMessageSetter is just an interface and a way to get around the new limitations of jdk16+, it's not meant to be a base exception class.
| if (!exceptionMessageHandlingLogged | ||
| if (detailMessageField == null | ||
| && !exceptionMessageHandlingLogged | ||
| && !ExceptionMessageSetter.class.isAssignableFrom(clazz)) { |
There was a problem hiding this comment.
should be BaseExceptionClass here no?
There was a problem hiding this comment.
No, this message is meant to warn the user when his baseExceptionClass does not implement ExceptionMessageSetter and that the reflection trick is not available
| } | ||
|
|
||
| public static class AdditionalNotInterfacedRuntimeException extends AbstractAdditionalRuntimeException { | ||
| public static class AdditionalNotInterfacedRuntimeException |
There was a problem hiding this comment.
something's up with the formatting no?
|
|
||
| public abstract class ServiceException extends BaseServiceException implements ExceptionMessageSetter { | ||
| public abstract class ServiceException extends BaseServiceException | ||
| implements ExceptionMessageSetter { |
There was a problem hiding this comment.
Same here. Why is it on a new line?
There was a problem hiding this comment.
the formatter is run by the fmt-maven-plugin, my guess is that the previous PR didn't run the formatting before merging?
There was a problem hiding this comment.
mcnoreau12
left a comment
There was a problem hiding this comment.
The formatting looks weirds but otherwise lgtm
The logging message when not being able to set accessible the
detailMessagefield was wrong so that's not fixed. The WARN is also not logged when the base exception implementsExceptionMessageSetter.Formatting has been fixed, last PR didn't run it it seems.