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 asciidoctor as manpage generator #1650

Merged
merged 2 commits into from Sep 11, 2018

Conversation

Projects
None yet
3 participants
@trws
Copy link
Member

trws commented Sep 11, 2018

This adds support for an alternate asciidoc translator. The unwritten docbook deps on a2x have been giving me heartburn on building a base docker image, this has a lot fewer deps (ruby, and... ruby AFAIK). May want to wait for the PR with a docker-based travis conf, which will be coming shortly.

fixes #873
Commit message with explanation below:

This allows asciidoctor to be used in place of a2x, which removes the
dependency on goodness-knows what docbook package our a2x generator
depends on that isn't in either our dependency list or theirs. This
exists primarily because after making three versions of a docker image
trying to get one that could build the docs the other way, this works
faster, with less deps, and seemingly the same result.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 11, 2018

Coverage Status

Coverage increased (+0.04%) to 79.585% when pulling f032f97 on trws:add_asciidoctor into 6091d38 on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Sep 11, 2018

Great!

Looks like the fix for the multiple manpage issue discussed in #873 went in at asciidoctor 1.5.7, so we should make that the minimum supported version or at least document it somewhere. The required version may not be in common circulation yet, e.g. Ubuntu 18.04LTS is still at 1.5.5.

The section 3 title change from "Flux Programming Reference" to "Flux Command Reference" should be dropped (it was correct before). Section 7 is wrong though - should be "Flux Miscellaneous Reference" or similar. Suggest any changes not related to asciidoctor go in a separate commit.

@trws trws force-pushed the trws:add_asciidoctor branch from b3ea4d9 to afc8516 Sep 11, 2018

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Sep 11, 2018

@trws sorry to be an irritation, but this PR should at minimum be split into two commits:

  • one that fixes the section 7 title
  • one that adds support for asciidoc
@trws

This comment has been minimized.

Copy link
Member Author

trws commented Sep 11, 2018

I don't mind fixing it up, but am not sure what change you'd like me to make. The PR is currently two commits, one that adds asciidoctor support and one that fixes the title and adds the dependency version to the readme. Is it that the addition to the readme should be in the other commit?

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Sep 11, 2018

First commit should not change the section 3 title, and second commit should not change it back. It didn't need to be changed, so that shouldn't be in the history.

You could squash the two commits to get rid of that incremental development, but it does seem like the change to the section 7 title for the non asciidoctor case should be in its own commit since it is topically distinct from the rest of the changes.

trws added some commits Sep 11, 2018

add support for asciidoctor as manpage generator
This allows asciidoctor to be used in place of a2x, which removes the
dependency on goodness-knows what docbook package our a2x generator
depends on that isn't in either our dependency list or theirs.  This
exists primarily because after making three versions of a docker image
trying to get one that could build the docs the other way, this works
faster, with less deps, and seemingly the same result.
fix section titles
Section 7 manpage title didn't make sense, changed as per request.

@trws trws force-pushed the trws:add_asciidoctor branch from afc8516 to f032f97 Sep 11, 2018

@trws

This comment has been minimized.

Copy link
Member Author

trws commented Sep 11, 2018

Fair enough @garlick, I think this version cleans that up, and the tests now passed after a retry.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Sep 11, 2018

Looks great, thanks.

@garlick garlick merged commit 1209a02 into flux-framework:master Sep 11, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.04%) to 79.585%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.