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.

Sign up for GitHub

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

Jump to bottom

Include more class in documentation #22011

Closed
wants to merge 1 commit into from
Closed

Include more class in documentation #22011

wants to merge 1 commit into from

Conversation

delongmeng-aws
Copy link

@delongmeng-aws delongmeng-aws commented Dec 8, 2021 •
edited by geoand
Loading

Simply checking if the java file contains strings like "extends BuildItem {", "extends SimpleBuildItem {", etc.
This extends the build items to include build items not ending with "BuildItem", such as "Capabilities.java", "CapabilityAggregationStep.java", and "ProcessInheritIODisabled.java".

Fixes: #17770

@quarkus-bot

This comment has been minimized.

Sign in to view
@quarkus-bot quarkus-bot bot added the area/infra-automation anything related to CI, bots, etc. that are used to automated our infrastructure label Dec 8, 2021
@delongmeng-aws delongmeng-aws changed the title fixing issue 17770 to include more build items Fixing issue 17770 to include more build items Dec 8, 2021
@delongmeng-aws delongmeng-aws mentioned this pull request Dec 8, 2021
Closed
@geoand geoand requested a review from gastaldi December 8, 2021 10:31
@geoand geoand changed the title Fixing issue 17770 to include more build items Include more class in documentation Dec 8, 2021
gastaldi
gastaldi reviewed Dec 8, 2021
View reviewed changes
Copy link
Contributor

@gastaldi gastaldi left a comment

Choose a reason for hiding this comment

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

That makes sense. I added some comments.

Another solution is to check if source.getSuperType() is one of those types from https://github.com/quarkusio/quarkus/blob/main/.github/quarkusbuilditemdoc.java#L107

.github/quarkusbuilditemdoc.java Outdated Show resolved Hide resolved
.github/quarkusbuilditemdoc.java Outdated Show resolved Hide resolved
gastaldi
gastaldi requested changes Dec 9, 2021
View reviewed changes
.github/quarkusbuilditemdoc.java Outdated Show resolved Hide resolved
.github/quarkusbuilditemdoc.java Outdated Show resolved Hide resolved
@gastaldi
Copy link
Contributor

gastaldi commented Dec 9, 2021

Can you squash all your commits into a single one? That would be necessary before merging.

Thanks!

@delongmeng-aws
Copy link
Author

Can you squash all your commits into a single one? That would be necessary before merging.

Thanks!

Yes let me see how I squash commits

@delongmeng-aws
Copy link
Author

Can you squash all your commits into a single one? That would be necessary before merging.
Thanks!

Yes let me see how I squash commits

I think it worked @gastaldi

gastaldi
gastaldi previously approved these changes Dec 9, 2021
View reviewed changes
@geoand
Copy link
Contributor

geoand commented Dec 9, 2021

Can you please squash the commits?

Thanks

gsmet
gsmet requested changes Dec 9, 2021
View reviewed changes
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Do we know if it will slow down the build significantly? Because AFAICS, we are now reading all the files and that could be very time consuming?

Until we have a better knowledge of this, I would like us to not merge it.

@gastaldi
Copy link
Contributor

gastaldi commented Dec 9, 2021

Do we know if it will slow down the build significantly? Because AFAICS, we are now reading all the files and that could be very time consuming?

Until we have a better knowledge of this, I would like us to not merge it.

That's a good point, perhaps it's easier to hardcode the missing items here instead (the ones that do not end with BuildItem.java)

@gastaldi
Copy link
Contributor

gastaldi commented Dec 9, 2021

In parallel, I think we should also consider renaming classes that do not follow this pattern (eg. ProcessInheritIODisabledto ProcessInheritIODisabledBuildItem)

@gsmet
Copy link
Member

gsmet commented Dec 9, 2021

@gastaldi yeah I think you're on the right track. I would special case the few that make sense and we want to keep and fix the others. I don't have time to drive this so I'll let you see with the OP :)

@gastaldi gastaldi mentioned this pull request Dec 9, 2021
Merged
@gastaldi
Copy link
Contributor

gastaldi commented Dec 9, 2021

@delongmeng do you have a list of the extra build items covered by this change?

@gastaldi gastaldi dismissed their stale review December 9, 2021 17:57

Dismissing approval, since reading every file contents will impact negatively in the build

gastaldi
gastaldi requested changes Dec 9, 2021
View reviewed changes
Copy link
Contributor

@gastaldi gastaldi left a comment

Choose a reason for hiding this comment

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

Just realized that this is not going to work because inner classes also contain extends XXXBuildItem { and they are not used (or supposed to be used) in the process method

@delongmeng-aws
fixing issue 17770 to include more build items, used a keyword list o…
8c1905f 
…f the build items and check if that's included in the files

moved keywordList to outside of the for loop, commented out original version, refactored variable name file to path, passed fileContent to process and Roaster.parse

Update .github/quarkusbuilditemdoc.java

Co-authored-by: George Gastaldi <gegastaldi@gmail.com>

Update .github/quarkusbuilditemdoc.java

Co-authored-by: George Gastaldi <gegastaldi@gmail.com>

remove the comment for the old version

moved KEYWORDS to class attribute and added back {

remove unnecessary package (Arrays)

removed duplicated package (List)
@delongmeng-aws
Copy link
Author

@delongmeng do you have a list of the extra build items covered by this change?

I saw three additional files: Capabilities, CapabilityAggregationStep, and ProcessInheritIODisabled, when I limited the directories to /core/deployment and /core/test-extension.

Saw the conversations above. Yes if this doesn't affect a large scope of files it might be easier to just follow the naming convention.

@gastaldi
Copy link
Contributor

I don't think CapabilityAggregationStep is intended to be used in other implementations. Given that ProcessInheritIODisabledis now deprecated in favor of ProcessInheritIODisabledBuildItem, Capabilities seems to be the only missing item. Can you hardcode that in the class instead?

@delongmeng-aws
Copy link
Author

I don't think CapabilityAggregationStep is intended to be used in other implementations. Given that ProcessInheritIODisabledis now deprecated in favor of ProcessInheritIODisabledBuildItem, Capabilities seems to be the only missing item. Can you hardcode that in the class instead?

Got it. I can definitely hardcode that :)

@gsmet
Copy link
Member

gsmet commented Dec 18, 2021

@delongmeng can you have another look at that? Thanks!

@gsmet
Copy link
Member

gsmet commented Jan 12, 2022

Closing this one as we will need a simpler patch. If you still want to work on this, let's do it in a new PR.

@gsmet gsmet closed this Jan 12, 2022
@quarkus-bot quarkus-bot bot added the triage/invalid This doesn't seem right label Jan 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gsmet gsmet gsmet requested changes

@gastaldi gastaldi gastaldi requested changes

Assignees
No one assigned
Labels
area/infra-automation anything related to CI, bots, etc. that are used to automated our infrastructure triage/invalid This doesn't seem right
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Some build items are missing in documentation
4 participants
@delongmeng-aws @gastaldi @geoand @gsmet