Skip to content
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

Only wrap exceptions when necessary in ArC subclassing #333

Merged
merged 2 commits into from
Dec 18, 2018
Merged

Only wrap exceptions when necessary in ArC subclassing #333

merged 2 commits into from
Dec 18, 2018

Conversation

gsmet
Copy link
Member

@gsmet gsmet commented Dec 18, 2018

This way, the proxy has an exception handling as similar as possible to
the original class.

Fixes #329.

My initial inclination was to add the test in SubclassGeneratorTest but I couldn't figure out how to properly initialize my bean (and ended up with a lot of boilerplate code reimplementing a lot of logic). So I ended up moving the test in arc-tests to have the test infrastructure handy.

This way, the proxy has an exception handling as similar as possible to
the original class.

Fixes #329.
@gsmet gsmet requested a review from mkouba December 18, 2018 10:34
Copy link
Contributor

@mkouba mkouba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise looks good!

@@ -65,6 +65,9 @@
*/
public class SubclassGenerator extends AbstractGenerator {

private static final DotName JAVA_LANG_EXCEPTION = DotName.createSimple(Exception.class.getName());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pls use org.jboss.protean.arc.processor.DotNames.create(Class<?>) instead - this constructs a componentized DotName which is more efficient when comparing names from an index.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to know, added a commit to fix that.

  • I took Throwable properly into account, even if not strictly necessary.

Also take into account Throwable, even if not critical - it works OK
without it.
@stuartwdouglas stuartwdouglas merged commit 4ffaf66 into quarkusio:master Dec 18, 2018
@cescoffier cescoffier added this to the 0.2.0 milestone Dec 18, 2018
maxandersen added a commit to maxandersen/quarkus that referenced this pull request Nov 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants