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 support for Asciidoc in configuration reference documentation #3887

Merged
merged 9 commits into from
Sep 10, 2019

Conversation

gsmet
Copy link
Member

@gsmet gsmet commented Sep 5, 2019

/cc @emmanuelbernard @FroMage @machi1990

Here it is. It's a bit more than a one liner as I had to restructure the parser and the output a bit. It fixes a logic issue anyway so it's all for good.

I haven't written the extension guide documentation yet but let's agree on this first.

One thing that is a bit less nice compared to the handcrafted output is that we lost the nice titled sections but I think having everything generated is worth it.

At some point, we could imagine a new tag to generate proper sections but let's do that another day.

Once Asciidoclet has the related patch, we will have a proper output in the generated Javadoc too.

@machi1990 could you upgrade your repository with this PR and point to it so that Emmanuel can have a look?

Note that GitHub does not support the new Font Awesome icons, I will attach a few screenshots.

Summary table

picture1

Details with Asciidoc

picture2

machi1990 added a commit to machi1990/quarkus-generated-extension-config-docs that referenced this pull request Sep 5, 2019
@machi1990
Copy link
Member

@machi1990 could you upgrade your repository with this PR and point to it so that Emmanuel can have a look?

Repository update - https://github.com/machi1990/quarkus-generated-extension-config-docs/blob/master/src/main/asciidoc/generated/quarkus-hibernate-orm.adoc

Note that GitHub does not support the new Font Awesome icons, I will attach a few screenshots.

Good thing you attached screenshots. GH did not like the icons :-P

*
* `drop-and-create` is awesome in development mode.
*
* Accepted values: `none`, `create`, `drop-and-create`, `drop`, `update`. The default is `none`.
*/
@ConfigItem
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a defaultValue="none" to the annotation so that it is nicely added to the summary table :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I have to tweak things a bit regarding that. And wondering if we could use the enum directly now but I have to test it. It's on my list of things to improve.

Copy link
Member

Choose a reason for hiding this comment

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

An enum would be even better.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, I'm just not sure it will fly, I need to check it and wanted this PR out of the door.

I will fix the default value though. Small improvements are better than later improvements :).

Copy link
Member Author

Choose a reason for hiding this comment

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

Done for the default value.

* Valid values are: none, first, last.
* Default precedence of null values in `ORDER BY` clauses.
*
* Valid values are: `none`, `first`, `last`.
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we replace this String type with an enum?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, see the discussion with Manyanda above. But I need to check if we can use an ORM one or if we have to create a new one so I consider it outside of the scope of this PR whose main purpose is to validate the Asciidoc support.

Copy link
Member

Choose a reason for hiding this comment

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

Ah sorry I didn't see it. Sure it can come in another PR.

@gsmet
Copy link
Member Author

gsmet commented Sep 6, 2019

I improved the output a bit by adding a light separator between each entry of the detailed output. Things are far more readable this way.

machi1990 added a commit to machi1990/quarkus-generated-extension-config-docs that referenced this pull request Sep 6, 2019
@machi1990
Copy link
Member

I improved the output a bit by adding a light separator between each entry of the detailed output. Things are far more readable this way.

tracking progress repo updated

Asciidoc comment are passthrough but we needed to restructure the parser
to enable proper parsing.
The Javadoc parser we use uses platform specific EOL characters.

This should fix the Windows CI failure.
Note that we will have to tweak the website CSS to keep it light.
The space just after the star is kept by the javax.lang.model API so
let's remove it.
We can't have the generated files in docs/target as it is cleaned prior
to generating the documentation when doing a mvn clean install.

Fixes quarkusio#3909
@emmanuelbernard emmanuelbernard merged commit 9b50bb7 into quarkusio:master Sep 10, 2019
@emmanuelbernard
Copy link
Member

Boom, its in!

@emmanuelbernard emmanuelbernard added this to the 0.22.0 milestone Sep 10, 2019
machi1990 added a commit to machi1990/quarkus that referenced this pull request Sep 10, 2019
following quarkusio#3909 docs are now generated in the global /target/ folder and not in
docs/src/main/asciidoc/generated. See this comment
quarkusio#3909 (comment)
and corresponding PR quarkusio#3887
machi1990 added a commit to machi1990/quarkus that referenced this pull request Sep 10, 2019
following quarkusio#3909 docs are now generated in the global /target/ folder and not in
docs/src/main/asciidoc/generated. See this comment
quarkusio#3909 (comment)
and corresponding PR quarkusio#3887
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants