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

Introduce AutoAddScopeBuildItem to remove boilerplate necessary when annotation transformers are used to add a scope annotation to a class #11350

Merged
merged 1 commit into from
Aug 13, 2020

Conversation

mkouba
Copy link
Contributor

@mkouba mkouba commented Aug 12, 2020

...annotation transformers are used to add a scope annotation to a class

@mkouba mkouba requested review from gsmet and manovotn August 12, 2020 14:43
@boring-cyborg boring-cyborg bot added area/arc Issue related to ARC (dependency injection) area/scheduler area/vertx labels Aug 12, 2020
*
* @return self
*/
public Builder hasInjectionPoint() {
Copy link
Member

Choose a reason for hiding this comment

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

It might be a stupid question and sorry if it's the case but should we also detect the lifecycle annotations? You might not have anything injected but a @PostConstruct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not a stupid question but I'm not sure all the specs support the lifecycle callbacks... In any case, we could add one more method to detect @PostConstruct and @PreDestroy.

Copy link
Member

Choose a reason for hiding this comment

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

Well, that wouldn't hurt the ones that don't support it and would be nice for the ones that do? Or am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I get the point. We should rename the method then. It's only best effort anyway because even though we walk the class hierarchy we don't handle some special cases (e.g. an overriden initializer method)...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, what about requiresContainerServices()?

@mkouba mkouba requested a review from gsmet August 13, 2020 06:54
Copy link
Contributor

@manovotn manovotn left a comment

Choose a reason for hiding this comment

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

Looks good, just one question.
It looks (maybe I am just missing something though) like this can lead to an erroneous state where you attempt to auto add scope but then later on ArC figures the given class is not a legal bean type. E.g. say it contains a wilcard.
I suppose in such case we blow up. Is that ok or should we add some checks for that?

@mkouba
Copy link
Contributor Author

mkouba commented Aug 13, 2020

@manovotn Yes, it could blow up and even the current approach could blow up. Maybe we could add some check later. In any case, it would be easier to fix it in one place ;-).

@gsmet
Copy link
Member

gsmet commented Aug 13, 2020

Very nice, let's get this in :).

@mkouba mkouba marked this pull request as ready for review August 13, 2020 09:04
@gsmet
Copy link
Member

gsmet commented Aug 13, 2020

Formatting issue:

2020-08-13T09:09:12.9478980Z [ERROR] Failed to execute goal net.revelc.code.formatter:formatter-maven-plugin:2.12.1:validate (default) on project quarkus-scheduler-deployment: File '/home/runner/work/quarkus/quarkus/extensions/scheduler/deployment/src/main/java/io/quarkus/scheduler/deployment/SchedulerProcessor.java' has not been previously formatted.  Please format file and commit before running validation! -> [Help 1]
2020-08-13T09:09:12.9499182Z org.apache.maven.lifecycle.LifecycleExecutionException: Failed to execute goal net.revelc.code.formatter:formatter-maven-plugin:2.12.1:validate (default) on project quarkus-scheduler-deployment: File '/home/runner/work/quarkus/quarkus/extensions/scheduler/deployment/src/main/java/io/quarkus/scheduler/deployment/SchedulerProcessor.java' has not been previously formatted.  Please format file and commit before running validation!

A mvn process-sources is in order!

@gsmet gsmet changed the title Introduce AutoAddScopeBuildItem to remove boilerplate necessary when... Introduce AutoAddScopeBuildItem to remove boilerplate necessary when annotation transformers are used to add a scope annotation to a class Aug 13, 2020
@mkouba
Copy link
Contributor Author

mkouba commented Aug 13, 2020

@gsmet Ah, damn -quickly again! Although I like the fact that Quarkus code is formatted automatically I don't like the current workflow :-(

...annotation transformers are used to add a scope annotation to a class
- also fixes quarkusio#11340
@gsmet
Copy link
Member

gsmet commented Aug 13, 2020

@mkouba could you check how much time it adds to enable the formatter with quickly?

@mkouba
Copy link
Contributor Author

mkouba commented Aug 13, 2020

@gsmet I don't get consistent results on my laptop :-(. It's roughly 20 seconds (03:32 min VS 03:14 min) if no formatting changes are performed.

BTW those failures are CI problems again:

Can't find any online and idle self-hosted runner in current repository that matches the required labels: 'ubuntu-latest'
Can't find any online and idle self-hosted runner in current repository that matches the required labels: 'windows-latest'

@gsmet gsmet merged commit a644d46 into quarkusio:master Aug 13, 2020
@gsmet gsmet added this to the 1.8.0 - master milestone Aug 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/arc Issue related to ARC (dependency injection) area/scheduler area/vertx
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JSON-B type adapter can't inject CDI beans
3 participants