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

Document that ContextConfigurator#done() is invoked automatically #33404

Merged
merged 1 commit into from
May 16, 2023

Conversation

manovotn
Copy link
Contributor

Note that this doesn't change or fix any behavior since there is a safety latch that already did this invocation in ArcProcessor, see this code.
Purpose of this PR is just to add clarity in how to properly operate with context configurators.

@manovotn manovotn added the area/arc Issue related to ARC (dependency injection) label May 16, 2023
@manovotn manovotn requested a review from Ladicek May 16, 2023 10:05
@Ladicek Ladicek requested a review from mkouba May 16, 2023 10:07
Ladicek
Ladicek previously approved these changes May 16, 2023
Copy link
Contributor

@Ladicek Ladicek left a comment

Choose a reason for hiding this comment

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

I'd prefer to keep it all as a single statement, as that's how builders are typically used, but that's just a personal preference. @mkouba WDYT?

@mkouba
Copy link
Contributor

mkouba commented May 16, 2023

I wonder if we should just add a note to the ContextConfiguratorBuildItem that done() is called automatically. That was probably one of the reasons why we introduced the ContextConfiguratorBuildItem anyway...

I'd prefer to keep it all as a single statement, as that's how builders are typically used, but that's just a personal preference. @mkouba WDYT?

👍

@github-actions
Copy link

github-actions bot commented May 16, 2023

🎊 PR Preview e2c3b4e has been successfully built and deployed to https://quarkus-pr-main-33404-preview.surge.sh/version/main/guides/

@manovotn manovotn force-pushed the customContextDoneInvocation branch from e339f68 to dbcb143 Compare May 16, 2023 11:10
@manovotn
Copy link
Contributor Author

I'd prefer to keep it all as a single statement, as that's how builders are typically used, but that's just a personal preference.

WDYM? The done() method returns void which prevents you from doing something like:

 return new ContextConfiguratorBuildItem(contextRegistrationPhase.getContext()
                .configure(TransactionScoped.class).normal().contextClass(TransactionContext.class).done());

@manovotn
Copy link
Contributor Author

I wonder if we should just add a note to the ContextConfiguratorBuildItem that done() is called automatically.

Right, we could do that instead but I found it sort of weird since all other Arc related configurators have done() method and require you to invoke it 🤷
I don't mind either way as long as it clarifies what is happening/should happen.

That was probably one of the reasons why we introduced the ContextConfiguratorBuildItem anyway...

I don't recall this but it's very well possible :)

@mkouba
Copy link
Contributor

mkouba commented May 16, 2023

Right, we could do that instead but I found it sort of weird since all other Arc related configurators have done() method and require you to invoke it shrug
I don't mind either way as long as it clarifies what is happening/should happen.

There are other places where we already call done() automatically: https://github.com/quarkusio/quarkus/blob/main/independent-projects/arc/processor/src/main/java/io/quarkus/arc/processor/AnnotationsTransformer.java#L372-L380 ;-)

@Ladicek
Copy link
Contributor

Ladicek commented May 16, 2023

The done() method returns void

Ouch, totally forgot about that. Well, that might be another reason why we call done() automatically :-)

@manovotn
Copy link
Contributor Author

There are other places where we already call done() automatically: https://github.com/quarkusio/quarkus/blob/main/independent-projects/arc/processor/src/main/java/io/quarkus/arc/processor/AnnotationsTransformer.java#L372-L380 ;-)

That's good pointer, I didn't realize we're already doing it. In that case, I'll scratch this and instead mention it's done automatically.

@manovotn manovotn force-pushed the customContextDoneInvocation branch from dbcb143 to cbcfc3b Compare May 16, 2023 13:31
@quarkus-bot quarkus-bot bot added the area/dependencies Pull requests that update a dependency file label May 16, 2023
@manovotn manovotn force-pushed the customContextDoneInvocation branch from cbcfc3b to 6396358 Compare May 16, 2023 13:34
@manovotn manovotn dismissed Ladicek’s stale review May 16, 2023 13:34

No longer relevant due to changes

@manovotn manovotn changed the title Custom context configurators should always invoke done() Document that ContextConfigurator#done() is invoked automatically May 16, 2023
@manovotn manovotn requested a review from Ladicek May 16, 2023 13:35
@manovotn
Copy link
Contributor Author

@mkouba @Ladicek I've changed the PR so that it now instead documents that ContextConfigurator#done() is invoked automatically.

@mkouba mkouba added triage/waiting-for-ci Ready to merge when CI successfully finishes and removed area/undertow labels May 16, 2023
@mkouba mkouba removed area/narayana Transactions / Narayana area/dependencies Pull requests that update a dependency file labels May 16, 2023
@quarkus-bot
Copy link

quarkus-bot bot commented May 16, 2023

✔️ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

@manovotn manovotn merged commit a75acb1 into quarkusio:main May 16, 2023
@manovotn manovotn deleted the customContextDoneInvocation branch May 16, 2023 22:00
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label May 16, 2023
@quarkus-bot quarkus-bot bot added this to the 3.1 - main milestone May 16, 2023
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/documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants