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

Don't generate docs when "quickly" sys prop is set to true #10523

Merged
merged 1 commit into from Jul 28, 2020

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Jul 7, 2020

Follows up on: #10230

@geoand geoand requested a review from gsmet July 7, 2020 09:47
@Sanne Sanne self-requested a review July 7, 2020 10:53
@geoand geoand added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jul 7, 2020
@geoand geoand added this to the 1.7.0 - master milestone Jul 7, 2020
Copy link
Member

@machi1990 machi1990 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the followup!

@geoand
Copy link
Contributor Author

geoand commented Jul 7, 2020

Thanks for the review folks!

@gsmet
Copy link
Member

gsmet commented Jul 7, 2020

I'm a bit torn about that one.

The reason is that it's nice to be able to have a fast build with the documentation generated as it's the only way to get a clean doc generation without inclusion warnings hiding other potential issues.

So I wonder if I would keep it the way it is and let people add -DskipDocs if they want to disable it.

@geoand
Copy link
Contributor Author

geoand commented Jul 7, 2020

IMHO, having to specify -DskipDocs when doing -Dquickly, kind of defeats the purpose of setting the skipDocs property in the quickly profile.
Moreover, I am pretty sure that the most common usage of quickly is to build the code, not the docs.

@gsmet
Copy link
Member

gsmet commented Jul 7, 2020

Well the thing is that you also need to check the doc at some point.

If we are sure it won't end up with having a ton of PRs with doc issues then go ahead.

@geoand
Copy link
Contributor Author

geoand commented Jul 7, 2020

I'm obviously not sure, I just think that this caters for the most common use case

@geoand
Copy link
Contributor Author

geoand commented Jul 7, 2020

So I'll leave it up to you :)

@geoand
Copy link
Contributor Author

geoand commented Jul 8, 2020

Looked at this again this morning and I think it's very counter intuitive to have to add -DskipDocs to -Dquickly.
For me, when one needs to create the docs, they simply shouldn't use -Dquickly (which set the skipDocs property anyway)...

@geoand geoand removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jul 8, 2020
@gsmet
Copy link
Member

gsmet commented Jul 22, 2020

@geoand so what should they do? Because when checking the docs properly, you need to build the whole tree.

Anyway, I have my own aliases now as I don't think I will agree with all of you so feel free to do whatever you want with quickly :).

@geoand
Copy link
Contributor Author

geoand commented Jul 22, 2020

The way I see quicky being used, is in cases where code needs to be built, not docs.

What I would suggest for the docs, is exactly what you ended up doing, having your own aliases :)

@gsmet
Copy link
Member

gsmet commented Jul 28, 2020

@geoand either you merge it or I close it, your call! :)

@geoand
Copy link
Contributor Author

geoand commented Jul 28, 2020

I am in favor of merging, but CI seems to be stuck?

@geoand
Copy link
Contributor Author

geoand commented Jul 28, 2020

I just rebased, let's see what happens

@geoand geoand added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jul 28, 2020
@geoand geoand merged commit b31e13f into quarkusio:master Jul 28, 2020
@geoand geoand deleted the docs-quickly branch July 28, 2020 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/core triage/waiting-for-ci Ready to merge when CI successfully finishes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants