Fixed bug from #152. #154
Fixed bug from #152. #154
Conversation
|
What is the status of this pull request ? Regards, Kamil |
|
@KamilKrol ; can you rebase to force re-test please |
garethahealy
added
enhancement
needs-rebase
possible-bug
and removed
enhancement
labels
|
@KamilKrol ; any update? |
|
@garethahealy , done. Let me know if you need something additional before merging this pull request. Kamil |
|
@orange-buffalo ; can you review please |
|
@KamilKrol ; instead of using "expected=Exception", can you use the JUnit rule checker instead. As currently, MappingException is for everything, so we wouldn't know if this test was actually broken in the future. |
| @SuppressWarnings("unused") | ||
| @Test(expected = MappingException.class) | ||
| public void testByteMapsToEnumOutOfOrdinalRange() { | ||
| mapper = getMapper(new String[] { "enumMapping.xml" }); |
There was a problem hiding this comment.
There is no need to explicitly create an array when calling the method. With varargs it is possible just to provide "enumMapping.xml" as an argument, which makes code cleaner and more readable.
| * Test on a mapping from short types to enum. | ||
| */ | ||
| @SuppressWarnings("unused") | ||
| @Test(expected = MappingException.class) |
There was a problem hiding this comment.
Agree with @garethahealy that ExpectedExceptionRule is a better approach to test exceptions here.
| /** | ||
| * Test on a mapping from short types to enum. | ||
| */ | ||
| @SuppressWarnings("unused") |
There was a problem hiding this comment.
These suppressions should be removed and the reasons for the warnings should be fixed. There is no need to assign a result of mapper.map() if it is not used.
| return enumClass; | ||
| } | ||
|
|
||
| private <T extends Enum<T>> boolean checkIfOverridenEnum(Class<T> enumClass) { |
There was a problem hiding this comment.
Just a minor thing - the typo in Overriden, should be Overridden. It also would be great to have naming aligned in test code, i.e. from Overrided to Overridden.
| } | ||
|
|
||
| private <T extends Enum<T>> boolean checkIfOverridenEnum(Class<T> enumClass) { | ||
| return Enum.class.isAssignableFrom(enumClass.getSuperclass()) && !Enum.class.equals(enumClass.getSuperclass()); |
There was a problem hiding this comment.
Enum.class.isAssignableFrom(enumClass.getSuperclass()) is redundant. It is already guaranteed that this code will be executed only for enums (seeMappingUtils.isEnumType before mapEnum call).
|
@KamilKrol ; are you able to work on the suggested changes? |
|
@KamilKrol ; have you had chance to work on this? |
KamilKrol
force-pushed
the
master
branch
3 times, most recently
from
0c47261 to
003786c
Compare
Problem with overriden Enum serialization to Map.
|
@garethahealy , I fixed all comments from code review and committed latest version. |
|
@KamilKrol ; the OSGi failure was a test flake. |
|
LGTM |
garethahealy
added
bug
needs-code-review
and removed
possible-bug
work-required-to-accept-pr
labels
|
@garethahealy , perfect that this OSGI failure is understood. |
Problem with overriden Enum serialization to Map.
We added also tests confirming the problem.
Kamil Krol & Dreando