Merge typehandlers-jsr310 into the core. #974
Conversation
There was a problem hiding this comment.
Hi @harawata, There is no objection with your proposal(It's great proposal). But I do not understand a mechanism to resolve for code that depends on different JDK versions on compile time yet... Could you explain it ? (The mechanism for test code is understanding already!)
And I've added some trivial comments.
| import org.apache.ibatis.lang.UsesJava8; | ||
|
|
||
| /** | ||
| * @author Tomas Rohovsky |
There was a problem hiding this comment.
I think @since 3.4.5 is need. (The same applies to other classes)
| @@ -0,0 +1,62 @@ | |||
| /** | |||
| * Copyright 2016 the original author or authors. | |||
There was a problem hiding this comment.
I think changing a license header is need. (The same applies to other classes)
| /** | ||
| * Type Handler for {@link JapaneseDate}. | ||
| * | ||
| * @since 1.0.2 |
There was a problem hiding this comment.
I think changing version is need. (The same applies to other classes)
|
|
||
| import org.apache.ibatis.lang.UsesJava8; | ||
|
|
||
| @UsesJava8 |
There was a problem hiding this comment.
I think adding javadoc(@since and @author etc..) is need.
| public abstract class Java8TypeHandlersRegistrar { | ||
|
|
||
| public static void registerDateAndTimeHandlers(TypeHandlerRegistry registry) { | ||
| // since 1.0.0 |
There was a problem hiding this comment.
I think this comment is not need.
| } | ||
|
|
||
| @Test | ||
| public void testFor_v1_0_0() throws ClassNotFoundException { |
There was a problem hiding this comment.
I think changing method name and merge to one method is need.
| import static org.hamcrest.core.IsInstanceOf.*; | ||
|
|
||
| /** | ||
| * Tests for auto-detect type handlers of mybatis-typehandlers-jsr310. |
There was a problem hiding this comment.
I think changing this comment is need.
|
Why don't you check |
|
Thank you guys for the comments! =) @kazuki43zoo , @raupachz , |
|
@harawata Ah, I see. Was just a suggestion. Probably all a matter of personal preference. Like most things. |
|
@raupachz ;D Any suggestion is welcome! |
# Conflicts: # src/test/java/org/apache/ibatis/type/usesjava8/Jsr310TypeHandlerRegistryTest.java
|
+1 |
Merge typehandlers-jsr310 into the core.
typehandlers-jsr310 are for standard Java types and they belong to the core in the first place.
We didn't know how to include them in the core without breaking Java 6 compatibility, but we do now.
So I propose to merge them into the core.
It might be a little bit confusing at first, but I cannot think of any serious issue caused by this change.
Any thoughts?
Cc-ing: @emacarron @hazendaz @kazuki43zoo @trohovsky @raupachz