Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings
This repository was archived by the owner on Oct 10, 2025. It is now read-only.

Java 17 compatible update#18

Merged
jebeaudet merged 1 commit intomaincoveooss/feign-error-decoder:mainfrom
feature/java17coveooss/feign-error-decoder:feature/java17Copy head branch name to clipboard
Jun 22, 2022
Merged

Java 17 compatible update#18
jebeaudet merged 1 commit intomaincoveooss/feign-error-decoder:mainfrom
feature/java17coveooss/feign-error-decoder:feature/java17Copy head branch name to clipboard

Conversation

@jebeaudet
Copy link
Collaborator

Fix the illegal reflective access in favor of an interface.

Library bumps across the board.

Copy link
Collaborator

@fredboutin fredboutin left a comment

Choose a reason for hiding this comment

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

NIce :D

README.md Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

implements ExceptionMessageSetter ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good catch

S exceptionToBeThrown = exceptionsThrown.get(exceptionKey).instantiate();
detailMessageField.set(exceptionToBeThrown, getMessageFromResponse(apiResponse));
if (exceptionToBeThrown instanceof ExceptionMessageSetter) {
((ExceptionMessageSetter) exceptionToBeThrown)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we log something to let the devs know that the exception message will not be set otherwise?

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe during initialization?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

well, it's set to something and depending on how your constructors are set up, it might be the expected message.

I could log something once when parsing the exception, that's not a bad idea I'll try something

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've added a log that logs only once.

@@ -159,6 +185,11 @@
<target>1.8</target>
Copy link
Collaborator

Choose a reason for hiding this comment

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

c'est encore en java 8?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes there isnt much incentive to bump the Java version here

@@ -37,13 +37,12 @@ public abstract class ReflectionErrorDecoder<T, S extends Exception> implements
private static final Logger logger = LoggerFactory.getLogger(ReflectionErrorDecoder.class);
private static final List<Object> SUPPORTED_CONSTRUCTOR_ARGUMENTS =
Arrays.asList(
Copy link
Collaborator

Choose a reason for hiding this comment

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

List.of() ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Java 8!

if (!exceptionMessageHandlingLogged
&& !ExceptionMessageSetter.class.isAssignableFrom(clazz)) {
logger.warn(
"The class '{}' or its superclass(es) do not implement ExceptionMessageSetter, therefore the Throwable detailMessage field will not be set. This will be only logged once.",
Copy link
Collaborator

Choose a reason for hiding this comment

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

superclasse(s) ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

One superclass two superclasses no?


@SuppressWarnings({"resource", "unused"})
@RunWith(MockitoJUnitRunner.class)
@ExtendWith(MockitoExtension.class)
Copy link
Collaborator

Choose a reason for hiding this comment

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

👏🏻

Copy link
Collaborator

Choose a reason for hiding this comment

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

import org.junit.jupiter.api.Assertions;

Copy link
Collaborator

Choose a reason for hiding this comment

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

en fait je pense que tu px ajouter un static import

import static org.junit.jupiter.api.Assertions.assertThrows;

@jebeaudet jebeaudet changed the title Java 17 update Java 17 compatible update Jun 22, 2022
Fix the illegal reflective access in favor of an interface.

Library bumps across the board.
@jebeaudet jebeaudet merged commit 7e36a13 into main Jun 22, 2022
@jebeaudet jebeaudet deleted the feature/java17 branch June 22, 2022 15:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

Comments

Close sidebar
Morty Proxy This is a proxified and sanitized view of the page, visit original site.