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

helpers: Add --trace to asciidoctor args #3714

Merged
merged 3 commits into from Jul 21, 2017

Conversation

Projects
None yet
3 participants
@miltador
Contributor

miltador commented Jul 18, 2017

This will help to understand and fix errors by seeing stacktrace of an error.

@miltador miltador requested a review from bep as a code owner Jul 18, 2017

@bep bep requested a review from anthonyfok Jul 18, 2017

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Jul 18, 2017

Member

I'm by no means an expert here, but we do fallback to asciidoc if asciidoctor isn't available -- and I don't see that flag there.

Member

bep commented Jul 18, 2017

I'm by no means an expert here, but we do fallback to asciidoc if asciidoctor isn't available -- and I don't see that flag there.

@miltador

This comment has been minimized.

Show comment
Hide comment
@miltador

miltador Jul 18, 2017

Contributor

I think asciidoc will just ignore it. Or I can add a check to only add --trace for asciidoctor.

Contributor

miltador commented Jul 18, 2017

I think asciidoc will just ignore it. Or I can add a check to only add --trace for asciidoctor.

@anthonyfok

This comment has been minimized.

Show comment
Hide comment
@anthonyfok

anthonyfok Jul 18, 2017

Contributor

Hi @miltador,

I think asciidoc will just ignore it.

$ asciidoc --version
asciidoc 8.6.9
$ asciidoc --trace
asciidoc: illegal command options
$ echo $?
1

So asciidoc does complain about an unknown command-line option and does exit with an error.

Or I can add a check to only add --trace for asciidoctor.

Please do so. 👍 Thank you for your contribution! 😉

Contributor

anthonyfok commented Jul 18, 2017

Hi @miltador,

I think asciidoc will just ignore it.

$ asciidoc --version
asciidoc 8.6.9
$ asciidoc --trace
asciidoc: illegal command options
$ echo $?
1

So asciidoc does complain about an unknown command-line option and does exit with an error.

Or I can add a check to only add --trace for asciidoctor.

Please do so. 👍 Thank you for your contribution! 😉

helpers: Add --trace to asciidoctor args
This will help to understand and fix errors by
seeing stacktrace of an error.
@miltador

This comment has been minimized.

Show comment
Hide comment
@miltador

miltador Jul 18, 2017

Contributor

Thanks for a test! Added a check.

Will add a smarter check (WIP).

Contributor

miltador commented Jul 18, 2017

Thanks for a test! Added a check.

Will add a smarter check (WIP).

@miltador

This comment has been minimized.

Show comment
Hide comment
@miltador

miltador Jul 18, 2017

Contributor

Added smarter check for asciidoctor.
@anthonyfok it is ready for review now.

Contributor

miltador commented Jul 18, 2017

Added smarter check for asciidoctor.
@anthonyfok it is ready for review now.

@anthonyfok

Thank you @miltador!

One tiny problem though: If both AsciiDoc and Asciidoctor are installed, your code would choose AsciiDoc over Asciidoctor. I am pretty sure that is not what you intended, especially since you use Ascidoctor yourself, and also because previous Hugo code has always "favoured" Asciidoctor over AsciiDoc. :-)

Could you please take a look again? Many thanks!

@miltador

This comment has been minimized.

Show comment
Hide comment
@miltador

miltador Jul 21, 2017

Contributor

@anthonyfok, switched the order to first check for Asciidoctor and then for Asciidoc.

Contributor

miltador commented Jul 21, 2017

@anthonyfok, switched the order to first check for Asciidoctor and then for Asciidoc.

@anthonyfok

That was very quick! Thank you @miltador!
Looks good to me! :-)

@anthonyfok anthonyfok merged commit b60aa1a into gohugoio:master Jul 21, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@miltador miltador deleted the miltador-forks:asciidoctor-trace-errors branch Jul 21, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment