Improve DevEx for event handlers

[lppedd]

I find myself in a situation where creating a class to handle GitHub events results in multiple inspection warnings in IntelliJ IDEA.
Take for example this screenshot:

This works perfectly. Quarkus wires up PullRequestMilestoneCheck and injects the constructor dependencies, and this GitHub extension does its job.
However, the class and all its methods are marked as unused, there is no IDE navigation to the injected beans, and it is unclear which bean scope the extension assigns to the class.

This could be improved by:

1. mandating the container class be annotated with @Singleton or @ApplicationScoped. Transitively avoids an explicit open.
2. moving event annotations to the method level

@Opened
@Reopened
@Edited
@Synchronize
fun onPullRequestEvent(payload: GHEventPayload.PullRequest) {
  //
}

[lppedd]

Looking at the sources, I see that a @Singleton scope is used when no scopes are provided.

quarkus-github-app/deployment/src/main/java/io/quarkiverse/githubapp/deployment/GitHubAppProcessor.java

Lines 722 to 724 in df32dd8

	if (!BuiltinScope.isDeclaredOn(declaringClass)) {
	multiplexerClassCreator.addAnnotation(Singleton.class);
	}

IMO an explicit scope would both make it clearer for consumers, and remove the need for that snippet.

[gsmet]

I don't know. My personal opinion on this is that the IDE is incorrect and I'm not sure I want to change things to comply with IDE requirements.

You can add the @Singleton scope if you wish.

I'm not sure how moving the annotations to the method helps?

[lppedd]

My personal opinion on this is that the IDE is incorrect and I'm not sure I want to change things to comply with IDE requirements.

The problem here is how can an IDE know that the class becomes part of Quarkus DI, when there are no markers to make it obvious?
And this is valid for all IDEs that support CDI. I'm ok with adding @Singleton on my classes, but it's definitely nicer to streamline it.

I'm not sure how moving the annotations to the method helps?

This is probably more of a style opinion, but it is also based on what IDEs offer.

Most annotations in Quarkus, Spring, Micronaut (etc.), are applied to methods instead of parameters.
In this specific case the information you interpret under the hood ends up linking to methods, so why not have them on methods directly.

Plus, in IntelliJ IDEA method annotations can be used to forcefully suppress unused warnings.

[gsmet]

And this is valid for all IDEs that support CDI. I'm ok with adding @singleton on my classes, but it's definitely nicer to streamline it.

I'm not sure I understand "streamlining it"? It would make things harder for other people.

You can simply add the annotation yourself if that's what you prefer and all is fine.

Most annotations in Quarkus, Spring, Micronaut (etc.), are applied to methods instead of parameters.

Well, I would say the annotations are supposed to target what's the target. In this case, there's a clear link between the event and the injected payload.
Now I can hear that it would probably also make sense to have the annotation on the method.
It was an early design choice because it makes things a bit easier to check that the payload and the annotation is consistent. But yeah, your position is equally good IMO.

Maybe we could support both and have an error if we have annotations in both places. I definitely don't want to break existing applications :).

I think it would require quite a few changes. Would you be willing to have a look at it?

[gsmet]

Oh, also, thanks for the feedback. I'm definitely not discarding it, let's have an open discussion.

[lppedd]

I'm not sure I understand "streamlining it"? It would make things harder for other people.

I just mean allowing doing it in a single way after a deprecation period, when you'll switch to 3.x.
I prefer being explicit over expecting something by the Quarkus extension (in this case, I mean adding the scope annotation for me).
If I need to look at sources to understand which scope is automatically applied, I feel like there is something off.

Correct me if I'm wrong, but you also do some magic with @Observes and @ObservesAsync right?
So that an explicit annotation isn't required.

Maybe we could support both and have an error if we have annotations in both places

Yup that's what I was thinking about. Adding a new target to the annotations, and adjust the extension's processor.

I think it would require quite a few changes. Would you be willing to have a look at it?

I can have a look, maybe next weekend! I guess most changes will be in GitHubAppProcessor.
It's not an urgent change, but with the growing amount of code I keep thinking about it lol

[gsmet]

If I need to look at sources to understand which scope is automatically applied, I feel like there is something off.

Maybe I need to be clearer on the intention here. For me, you don't need a scope because you don't even need to care about CDI.
And your event listening classes are not really CDI beans btw. We veto them from CDI here: https://github.com/quarkiverse/quarkus-github-app/blob/main/deployment/src/main/java/io/quarkiverse/githubapp/deployment/VetoUserDefinedEventListeningClassesAnnotationsTransformer.java .

From these classes, we generate extra classes which will be CDI beans and wire everything (including the @Observes as you guessed it correctly). This is necessary because when listening for multiple events, you need separate methods for each type of event (CDI qualifiers are a AND, not a OR).

The thing is I don't really want people to care about CDI or require they understand all the wiring.

I can understand how it may feel a bit too magic when you actually are used to CDI.

I can have a look, maybe next weekend! I guess most changes will be in GitHubAppProcessor.

Yes, exactly.
My guess is that some logic will need to change but I don't think that would be so hard.

We definitely need to make sure we raise an error if we have annotations on both methods and parameters.