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

Add @see to bean suppression methods' javadoc to link them together #40716

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sixcorners
Copy link

Think this is a good idea?

@quarkus-bot quarkus-bot bot added the area/arc Issue related to ARC (dependency injection) label May 19, 2024
@sixcorners sixcorners changed the title Add @see to bean suppression methods to link them together Add @see to bean suppression methods' javadoc to link them together May 19, 2024
@gastaldi gastaldi requested a review from mkouba May 20, 2024 15:09
@@ -53,6 +55,9 @@
* </pre>
*
* @see Instance
* @see LookupUnlessProperty
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that it definitely makes sense to link the LookupUnlessProperty here, but I'm not so sure about IfBuildProperty/UnlessBuildProperty? And the same applies to LookupUnlessProperty...

@Ladicek @manovotn WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, agree. @IfBuildProperty and @LookupIfProperty look superficially similar, but they are very different (the first is build-time only, the other is runtime-aware but only applies to dynamic lookup). I wouldn't add a @see reference to them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, LookupIfProperty and LookupUnlessProperty should reference each other a so should IfBuildProperty/UnlessBuildProperty but I wouldn't cross reference them.

Copy link
Author

Choose a reason for hiding this comment

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

Would it make sense to change it to a sentence that links them together but also states the difference? I kind of feel like someone looking for one might end up looking at the docs for the other.

Copy link
Contributor

Choose a reason for hiding this comment

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

Both of these concepts are explained within the same document - https://quarkus.io/guides/cdi-reference
To me the difference is clear enough there but I am definitely biased as I am familiar with CDI as well as and many of these concepts. If you feel like you can add a sensible comparison there, it might be better than the javadoc?

Copy link
Author

Choose a reason for hiding this comment

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

I know they are explained on the website but putting the information in the javadoc can help when you are just asking your IDE to bring up the annotation name you kind of remember and you wind up at the wrong one.

see doesn't mean that the linked thing does the same thing. NormalScope for instance references Scope despite them working differently. When I look at the javadoc of something and it points at something that sounds the same my assumption would be the opposite of thinking they work the same way.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, the problem is that those annotations not only "work differently", they are used in different phases of the app lifecycle. IfBuildProperty is considered at build time whereas the LookupIfProperty is used during programmatic lookup at runtime. So I think that a simple @see reference does not really help but instead can be confusing for users.

I think that we should either add a paragraph like "If you need to control the enablement of a bean at build time then use..." or skip the build time annotations completely.

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)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants