#2730 Add support for Jakarta XML Binding #2994
Conversation
ibogdanchikov
force-pushed
the
jakarta-xml-binding-support
branch
2 times, most recently
from
d2228ee to
5253006
Compare
filiphr
left a comment
filiphr
left a comment
There was a problem hiding this comment.
Thanks a lot for your effort in this @ibogdanchikov.
I've left some comments inline. I am mostly concerned about some of the changes in the pom.xml files.
Let us know if you are busy and don't have time to finish this. We can take over and bring it over the finish line.
processor/src/main/java/org/mapstruct/ap/internal/model/source/builtin/JaxbElemToValue.java
Show resolved
Hide resolved
|
@filiphr Thank you for the code review and your comments. Sorry to disappoint you with the quality of my work. I will do my best to improve it. Concerning my free time, it is better now, so I will definitely finish this. |
@ibogdanchikov you should not look at code reviews in such a manner. There must have been a reason for some of the things, and I wanted to understand them. I view code reviews as a chance to learn and grow, both when I am the reviewer and when I submit a PR. |
I understand what you mean, but I can assure you that I don't and never did (I've been through hundreds of code reviews by now).
As, of course, do I. Perhaps I should have phrased it better. I was merely acknowledging that I could have done a better job overall. I am, for one, disappointed that I didn't realize from the beginning that Jakarta XML Binding is only supported by JDK versions higher than 8 (bytecode wise), so now the tests are failing for the JDK 8. |
Have a look at 4708f4b. It happened to me few days ago 😉. Anyways, what I would suggest for this is to to something similar like in that PR. You will also need to add more excludes in |
|
Sorry for closing the PR. I pressed the wrong combination of buttons and it closed. I've reopened it. |
|
@ibogdanchikov I took the liberty to do some small changes to your PR. Have a look at them and let me know what you think. It is mostly for removing some duplicated code and cleaning up a bit of where the |
ibogdanchikov
force-pushed
the
jakarta-xml-binding-support
branch
from
53b5277 to
980cae1
Compare
|
First of all, I wanted to clear out one thing I wrote in this comment. I was once again wrong about this bit:
The version @filiphr, thanks for the advice. I didn't get to apply it in the end, because I was wrong about the Jakarta XML Binding needing >Java9, as I wrote above. @filiphr, I responded to all of the comments you've left so far. I also pushed the updated code. I would like to ask for another round of CR, if that's possible. Thank you. |
ibogdanchikov
changed the title
|
Thanks a lot for all the changes @ibogdanchikov. Everything looks great, I just merged this to main |
|
@filiphr Thank you! |
2 participants
Adds support for Jakarta XML Binding. See #2730 for details.
Sorry it took me so long (took me a while to figure out dependencies + I unfortunately didn't have much free time).
@filiphr, for now I have left the work split into a few commits so the code review could be done more easily, and so the logic of the changes could be seen more clearly. I will squash the commits into one after the code review. But if you would prefer I can squash them right away.
Thank you.
Fixes #2730