Hello mybatis!
I am a user of mybatis. I used to post a question to mybatis google groups in the link below.
https://groups.google.com/forum/#!topic/mybatis-user/iVNejGgsAP4
And as an answer, I took a link to the 1126 issue and tested it. https://github.com/mybatis/mybatis-3/issues/1126
However, the test code did not work. So, after I forked the project myself, I analyzed it.
As a result, I found that all queries written in xml are handled by SqlSourceBuilder. So, I will send you the source I modified. I respectfully ask for a review. Thank you.
Comment From: hazendaz
@harawata This LGTM. Build is fine (unrelated jdk 15 issue only). I think this is a pretty clean implementation and good reuse of code. I have had dba's complain about this as well. I can see the value here. I know you were involved with a lot of earlier comments. Do you think this is good to go?
Comment From: hazendaz
@elfhazard This looks good. I'm checking with @harawata before merging.
Comment From: elfhazard
@elfhazard This looks good. I'm checking with @harawata before merging.
@hazendaz Thank you for your review. Have a nice day!
Comment From: elfhazard
Oh~! JDK 15 build test is removed!! Thank you! My local jdk 15 test also failed. I think error on maven plug in.
Comment From: harawata
Thank you for the quick update, @elfhazard !
As this adds a new option, there are a few chores to do.
- Add documentation (i.e. edit
configuration.xml
insrc/site
directory) - Update XmlConfigBuilderTest
Could you work on them if you still have some spare time? It is totally fine if you are busy. I will do it myself later.
Regarding the documentation, only the English version is required, but please copy the English version to other languages' configuration.xml
to help translators.
Comment From: elfhazard
@hazendaz I added doc with two languages that is en and ko. Other languages is added as en. And XmlConfigBuilderTest is update. Have a good day!
Comment From: harawata
Thank you for the update, @elfhazard !
Regarding the doc, I have removed the term 'xml' from the explanation because this feature affects SQL in annotations as well. Could you re-translate the Korean version when you have the time, please?
@hazendaz , I'll merge this tomorrow, probably. Please let me know (or commit directly) if there is anything you noticed!
Comment From: hazendaz
@harawata Only thing that jumps out to me is the naming. Shrink whitespace vs removeExtraWhitespaces. I think those should be named the same to avoid confusing over the naming.
Comment From: elfhazard
@harawata I update korean description. Please check this commit. Thank you!
Comment From: harawata
Merged. Thank you for being super-responsive, @elfhazard !