-
Notifications
You must be signed in to change notification settings - Fork 2
Support POJO binding. #37
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice PR. Thanks 🎉
It lacks tests and changes in the documentation. I leave it to you to extend the PR also based on some inline comments I left on some smaller details. I'm also willing to merge the PR as is but I will follow it up with mentioned changes by myself.
btw the QA fails just because sonarcloud is unable to check the fork PR, let's disable that like
project.tasks['sonarqube'].enabled System.getenv('SONAR_TOKEN') != null
return buildPipeline().build(); | ||
T config = buildPipeline().build(); | ||
// Clean the class reflection cache. | ||
ClassToImplCache.clear(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I generally approve of making this cache static. The reason it was not was to avoid any kind of non-deterministic behaviour in case two configurations load concurrently plus have it all GC'd when done.
I think this clear isn't needed, but I'm aiming for having a WeakMap + a bit of synchronization sugar instead.
* @since 1.0.0, 2020-05-17 08:40 | ||
*/ | ||
@ToString | ||
public class PojoBinding<T> implements ConfigBinding<T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very nice code! i recognize quite some things of it 😉
I also see lots of duplication compared to the InterfaceBinding, which could be reduced by Pojo* classes extending from the Interface* onces and overriding small parts of it...
Let's also drop the authorship from the file. I think of adding a contributor section to the docs instead.
} | ||
|
||
protected MemberResolver supportedMemberResolver() { | ||
MemberResolver memberResolver = new MemberResolver(new TypeResolver()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here you could use the the common (caching) type resolver
MemberResolver memberResolver = new MemberResolver(Util.TYPE_RESOLVER);
@keykey7 Tks for your detailed review advice. I will work on this PR later tonight. |
11374c6
to
923272f
Compare
Fix the issue: #27