Generator extension API + jte-models extension #224
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #224 +/- ##
============================================
- Coverage 92.92% 91.58% -1.34%
+ Complexity 1128 1122 -6
============================================
Files 65 67 +2
Lines 2882 2901 +19
Branches 464 460 -4
============================================
- Hits 2678 2657 -21
- Misses 111 151 +40
Partials 93 93
☔ View full report in Codecov by Sentry. |
|
@edward3h that‘s awesome progress! I‘ll have a detailled look this weekend and proper feedback 😊 |
There was a problem hiding this comment.
Hey @edward3h I finally had the time to do a proper review. And wow, that looks great! I really like that it's a general purpose extension API, as the native image resources are now possible with an extension instead of hacking into the project. Also, I find it really funny that you're using jte to generate the models :-D.
I left a few notes in the code, mostly minor stuff. I have left a note on the JteModel interface, I have a gut feeling that the render() methods there need to be simplified a bit. I left a suggestion, let me know what you think about it!
Overall I feel very confident that we can merge this soon.
jte/pom.xml
Outdated
| <artifactId>jte-extension-api</artifactId> | ||
| <version>${project.version}</version> | ||
| </dependency> | ||
| <dependency> |
There was a problem hiding this comment.
Should we remove the native resources extension here? This way users who need it can add the dependency and others don't have it by default?
There was a problem hiding this comment.
The intention was to keep backward compatibility for users who have configured native resources with the existing boolean property. Maybe there is another way to do that. Do you have any ideas?
I can figure out how to do it in Gradle :)
There was a problem hiding this comment.
Maybe we could throw an Exception when the native resources boolean property is set to true? And put instructions into the exception message how to migrate to the new extension?
There was a problem hiding this comment.
I guess if we put this in 3.0 we can just drop the existing setting and be less concerned about compatibility.
There was a problem hiding this comment.
I have made this change - dropped all the uses of the native resources boolean property and updated tests to use the extension instead.
|
Yes I was playing with String.format to generate model classes, then thought it would be easier with some sort of template library, then "Oh, I have one right here". It is nice that it worked out. |
|
I pulled updates from main and got tests passing again. |
|
That looks really good! I don't see anything that should prevent us from merging :-) @edward3h what do you think? |
|
Looks good to me :) |
|
🎉 Thank you for this amazing extension API contribution @edward3h! |
This is what I've done for generating a 'model' facade. I need to finish some more documentation, but thought it would be good to share.