-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ArC - transform unproxyable bean classes by default #7837
Conversation
I can't really tell if the TCK failure is related or not... The error is rather obscure. Can you give it a check @mkouba ? |
That's a bug in the TCK: eclipse/microprofile-config#543 We do transform the ConfigObserver class but there is obviously some class loading problem, most probably related to our arquillian adapter. I think I'll try to disable the transformation for MP Config TCK fow now... |
OK, thanks |
extensions/arc/deployment/src/main/java/io/quarkus/arc/deployment/ArcConfig.java
Show resolved
Hide resolved
independent-projects/arc/processor/src/main/java/io/quarkus/arc/processor/BeanProcessor.java
Outdated
Show resolved
Hide resolved
...c/test/java/io/quarkus/arc/test/unproxyable/PrivateNoArgsConstructorDoNotChangeFlagTest.java
Show resolved
Hide resolved
I like it a lot, +1. The implementation also looks straight-forward but I'll let @manovotn comment on that since this is his wheelhouse :) |
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.
Looks really good, this actually fixes quite a few proxyability cases where we wouldn't pass TCK's exception expectations (deployment versus definition) :-)
Added just some minor questions/remarks.
extensions/arc/deployment/src/main/java/io/quarkus/arc/deployment/ArcConfig.java
Show resolved
Hide resolved
...t-projects/arc/processor/src/main/java/io/quarkus/arc/processor/BeanDeploymentValidator.java
Show resolved
Hide resolved
independent-projects/arc/processor/src/main/java/io/quarkus/arc/processor/BeanProcessor.java
Show resolved
Hide resolved
independent-projects/arc/processor/src/main/java/io/quarkus/arc/processor/BeanProcessor.java
Show resolved
Hide resolved
- new transformations: remove final flag from a bean class, change visibility of private no-args constructor - deprecate quarkus.arc.remove-final-for-proxyable-methods - introduce quarkus.arc.transfrorm-unproxyable-classes - this should also help kotlin developers where classes are final by default
- BeanProcessor.Builder.setRemoveFinalFromProxyableMethods()
Github UI is reporting a conflict |
@geoand Yep, just in the middle of rebasing and testing ;-). |
I am actually thinking that this type of transformation could be applied in other areas that affect Kotlin as well... |
I am not working in Kotlin so I cannot say how much of an issue is this. But if it's a biggie then I see no reason not to backport it. |
At the moment, Kotlin + Gradle (which is what most Kotlin users use), does not work nice at all in dev-mode. We always knew this, but after actually witnessing this in action I am convinced that this approach is great. |
+1 for the approach but I'm not so sure about backporting. (1) it's a breaking change. (2) we modified a lot of files so it might not be that easy to backport this PR. |
I see, in that case if we are not sure about the feasibility of backporting, then I won't insist :) |
If possible I would like to backport, but I understand that it might not be practical. |
visibility of private no-args constructor
default