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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor README into Antora site #498

Merged

Conversation

abelsromero
Copy link
Member

Thank you for opening a pull request and contributing to asciidoctor-maven-plugin!

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Documentation
  • Refactor
  • Build improvement
  • Other (please describe)

What is the goal of this pull request?

Goals are:

  • Migrate docs to antora for future integration in central Asciidoctor site.
  • Publish docs in local gh-pages to provide a rendered site in the meantime. Both publications are not explusive. The only change to do once the final Asciidoctor site is released officially is changing the docs link in the readme and disable gh-page.

NOTE: the UI bundle is in github.com/abelsromero/asciidoctor-maven-plugin-antora-ui/.
Won't deny some improvements could be made.

Are there any alternative ways to implement this?
N/A

Are there any implications of this pull request? Anything a user must know?
Full documentation won't be readable from GitHub UI.

Is it related to an existing issue?

Finally, please add a corresponding entry to CHANGELOG.adoc

@abelsromero abelsromero force-pushed the feature/migrate-readme-to-antora branch from 170a628 to dbd002f Compare November 22, 2020 18:20
@abelsromero abelsromero mentioned this pull request Nov 22, 2020
8 tasks
@abelsromero
Copy link
Member Author

abelsromero commented Nov 22, 2020

Final structure is

image

With 4 modules: ROOT(with index), plugin, site-integration, project.

Some decisitons:

  • The structure mimics the sections of other popular maven plugins (compiler, jar) so user should have no problems, even without text indexation.
  • I have taken inspiration from Asciidoctor.js and Intellij plugin for README and contributing sections.
  • Shared options for all plugins appear in all goal descriptions using partials inclusion intead of linking to each-other like we did in README.

Overall I think the documents are much more structure now. Imho, it gets to a point where a single document becomes hard to handle and it shows.

@coveralls
Copy link

coveralls commented Nov 22, 2020

Coverage Status

Coverage remained the same at 84.971% when pulling 314d98a on abelsromero:feature/migrate-readme-to-antora into fd97bc7 on asciidoctor:master.

@@ -0,0 +1,93 @@
[[auto-refresh-goal]]
Copy link
Member

Choose a reason for hiding this comment

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

Please use the shorthand anchor [#auto-refresh-goal] instead of the semi-deprecated block anchor.

The plugin has 3 goals.
None of these are bound to any specific phase.

* xref:goals/process-asciidoc.adoc[asciidoctor:process-asciidoc]:
Copy link
Member

Choose a reason for hiding this comment

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

I think this would look better as a dlist:

xref:goals/process-asciidoc.adoc[asciidoctor:process-asciidoc]::
main goal of the plugin, it is used to convert documents during a normal Maven build.

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 no problem.
On the topic, truth I am never sure when to use them. This is one of those things that I would appreciate to have in a style guide.


Combining it with logHandler option it is possible to report an error and abort a build in case of missing attributes.

[source, xml]
Copy link
Member

Choose a reason for hiding this comment

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

I recommend being consistent about using a space after the comma. I prefer it without a space, but as long as it's consistent, that's what really matters.

Copy link
Member Author

Choose a reason for hiding this comment

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

100% agree, thanks for pointing it

docs/antora.yml Outdated
@@ -0,0 +1,20 @@
name: maven-plugin
title: Asciidoctor Maven Plugin
Copy link
Member

Choose a reason for hiding this comment

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

I can't decide whether we should drop "Asciidoctor" from the component title. It seems redundant to say "Asciidoctor Maven Plugin" on the Asciidoctor docs site. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is just for the title at the top of the toc. If that's not going be displayed in the final site, I image I can remove it from the UI theme I am using. wdyt?
image

Also, technically speaking we should call the project "Maven Tools". The Maven plugin and the doxia site module are bundled together for historic reasons but they are two separated components in truth that should be packaged in different jars (maven sub-modules).

Copy link
Member

Choose a reason for hiding this comment

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

Seems very reasonable to name the docs that way using an umbrella term.

docs/antora.yml Outdated
release-version: 2.1.0
maven-site-plugin-version: 3.9.0
project-repo: asciidoctor/asciidoctor-maven-plugin
uri-repo: https://github.com/{project-repo}
Copy link
Member

Choose a reason for hiding this comment

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

You can't use attribute references in a component descriptor.

Copy link
Member Author

Choose a reason for hiding this comment

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

Luckily I see uri-repo is only required for the README, so we can remove it. Thanks!

docs/antora.yml Outdated
uri-examples: https://github.com/asciidoctor/asciidoctor-maven-examples
uri-maven: http://maven.apache.org
idprefix: ''
idseparator: '-'
Copy link
Member

Choose a reason for hiding this comment

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

The idprefix and idseparator should only be set in the playbook, not the component descriptor.

Copy link
Member Author

Choose a reason for hiding this comment

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

What should be the criteria? In practice it makes no diference right?

Copy link
Member

Choose a reason for hiding this comment

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

This would be part of the style guide. It will indicate which attributes should be managed globally. All others can be managed by the component version or page.

@@ -0,0 +1,11 @@
* Maven Plugin
** xref:introduction.adoc[Introduction]
Copy link
Member

Choose a reason for hiding this comment

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

You don't have to specify text in the xref macro. It will use the page title, which you can override using the navtitle attribute defined in the document header of that page.

Just open the file through the provided url that will appear in the console and write.

[IMPORTANT]
====
Copy link
Member

Choose a reason for hiding this comment

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

If an admonition is only a single paragraph, you don't need the block delimiters around it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed this and a couple of more cases.
I mantained one WARNING that has 4 senteces and is intertwined with the dlist of the parameters. I think the block makes reading it easier in the source.

When `true`, instead of generating all output in a single folder, output files are generated in the same structure.
See the following example
+
[source]
Copy link
Member

Choose a reason for hiding this comment

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

The style on a text-based diagram should be listing, not source (or you can make it a literal block).

Copy link
Member Author

Choose a reason for hiding this comment

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

I am going to got for literal.

@mojavelinux
Copy link
Member

Overall, this looks really great. I left some comments inline.

@mojavelinux mojavelinux changed the title Refactor README into Anota site Refactor README into Antora site Nov 23, 2020
@@ -0,0 +1,11 @@
* Maven Plugin
Copy link
Member

Choose a reason for hiding this comment

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

I recommend using the terminology "Main Plugin" here, or perhaps "Processor Plugin". Having "Maven Plugin" twice in the breadcrumbs feels confusing.

Copy link
Member Author

Choose a reason for hiding this comment

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

With the "Maven Tools" title renaming this gets solved quite nicely 馃槃

@abelsromero abelsromero force-pushed the feature/migrate-readme-to-antora branch from dbd002f to 106fe6b Compare November 23, 2020 23:14
@abelsromero
Copy link
Member Author

abelsromero commented Nov 23, 2020

@mojavelinux Thanks a lot for the thorough review. All suggestions made sense.

@mojavelinux
Copy link
Member

Thanks Abel. Great work!

@@ -0,0 +1,2 @@
* Site integration
Copy link
Member

Choose a reason for hiding this comment

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

When I look at the TOC, it really feels like this should say "Maven Site Integration" instead of just "Site Integration". We may understand the context well, but for someone looking for it, I think "Site Integration" is going to be too abstract to be understood.

Copy link
Member Author

Choose a reason for hiding this comment

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

2 observations:

  • It gets to a point in which I have read through the content so many times that words lose meaning to me 馃樀 I greatly appreaciate reviews like this.
  • If it seems Site sections is "shoe horned", it is. There's not enough content to make several pages, but I wanted to have a two level toc for consistency.

This is the final toc, I capitalized all titles for consistency

image

@abelsromero abelsromero force-pushed the feature/migrate-readme-to-antora branch 2 times, most recently from 94ee72a to 6c4147d Compare November 25, 2020 22:14
@abelsromero abelsromero force-pushed the feature/migrate-readme-to-antora branch from 6c4147d to 314d98a Compare November 29, 2020 15:07
@abelsromero
Copy link
Member Author

If there are no more comments, I will merge tomorrow.

@abelsromero abelsromero merged commit 0f4b15b into asciidoctor:master Nov 30, 2020
@abelsromero abelsromero deleted the feature/migrate-readme-to-antora branch April 10, 2021 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants