ggrossetie / asciidoctor-web-pdf

Convert AsciiDoc documents to PDF using web technologies
https://asciidoctor.org
MIT License
448 stars 92 forks source link

proper handling of --version and --help. #489

Closed wh81752 closed 3 years ago

wh81752 commented 3 years ago

This PR address proper handling of command-line options --version and --help:

"veto" super-power: When option --help or --version is used, then all other arguments are ignored. Option --version has higher priority than option --version (standard and expected behaviour; default behaviour of yargs anyway.

ggrossetie commented 3 years ago

@wh81752 I guess it would be better to fix this issue upstream (since we have the same issue with asciidoctor CLI): https://github.com/asciidoctor/asciidoctor-cli.js

wh81752 commented 3 years ago

@Mogztter - perhaps. Nontheless, it fixes problem of this project. Is there a problem merging?

ggrossetie commented 3 years ago

Yes, I don't want to fix this issue here, then upstream, then fix it again here (by reverting or adapting the code).

I've noticed that both --version and --help are missing from the usage on asciidoctor CLI. Since asciidoctor-web-pdf relies on asciidoctor CLI it would be more beneficial to fix it upstream.

For reference, asciidoctor-revealjs also relies on asciidoctor CLI.

I will try to understand why it's no longer working on asciidoctor CLI.

ggrossetie commented 3 years ago

It's a bit complicated because we are trying to mirror what Asciidoctor (Ruby) CLI is doing:

$ asciidoctor -v
Asciidoctor 2.0.10 [https://asciidoctor.org]
Runtime Environment (ruby 2.6.3p62 (2019-04-16 revision 67580) [x86_64-darwin18]) (lc:UTF-8 fs:UTF-8 in:UTF-8 ex:UTF-8)
$ asciidoctor -V
Asciidoctor 2.0.10 [https://asciidoctor.org]
Runtime Environment (ruby 2.6.3p62 (2019-04-16 revision 67580) [x86_64-darwin18]) (lc:UTF-8 fs:UTF-8 in:UTF-8 ex:UTF-8)
$ asciidoctor --version
Asciidoctor 2.0.10 [https://asciidoctor.org]
Runtime Environment (ruby 2.6.3p62 (2019-04-16 revision 67580) [x86_64-darwin18]) (lc:UTF-8 fs:UTF-8 in:UTF-8 ex:UTF-8)

The --help option is also tricky because we can do --help syntax which will show an overview of the AsciiDoc syntax:

$ asciidoctor --help syntax
= AsciiDoc Syntax
:icons: font
:stem:
:toc: left
:url-docs: https://asciidoctor.org/docs
:url-gem: https://rubygems.org/gems/asciidoctor

A brief reference of the most commonly used AsciiDoc syntax.
You can find the full documentation for the AsciiDoc syntax at {url-docs}.

//...

Having said that , we might want to drop the --help syntax feature in the asciidoctor-web-pdf CLI.

After all, it might be a safer option to fix this issue downstream first 😅

wh81752 commented 3 years ago

Yes, I don't want to fix this issue here, then upstream, then fix it again here (by reverting or adapting the code).

Hmm, the code is like this:

    const argsParser = this.options.argsParser()
      .version(new Invoker().version())
      .help()

    this.options.argsParser = () => {
      return argsParser
    }

where this.options is from @asciidoctor itself. All it does is overriding @asciidoctor's version() and help().

Having said that , we might want to drop the --help syntax feature in the asciidoctor-web-pdf CLI.

I'm not going to miss it. Some alternatives:

How shall we continue now?

ggrossetie commented 3 years ago

I guess with some tweaking yargs could learn to handle both, --help and --help syntax

I tried but wasn't successful. Yarg's --help expects a boolean, not a string. As far as I know, the only way to customize --help is to disable it with .help(false) but then the option won't show in the command usage.

I think we could open an issue upstream.

How shall we continue now?

I will checkout your branch and do some additional tests to make sure that we won't break anything.

ggrossetie commented 3 years ago

Nevermind, the definition order is important, I need to disable the default --version and --help option and then declare them:

.version(false)
.option('version', {
  alias: 'V',
  default: false,
  describe: 'display the version and runtime environment (or -v if no other flags or arguments)',
  type: 'boolean'
})
.help(false)
.option('help', {
  describe: `print a help message
  show this usage if TOPIC is not specified or recognized
  show an overview of the AsciiDoc syntax if TOPIC is syntax`,
  type: 'string'
})
wh81752 commented 3 years ago

@Mogztter - I'm sure you disagree on this, nonetheless: My general approach would be that asciidoctor-web-pdf is a toolset just using asciidoctor (as library) rather than deriving from asciidoctor.

In other words, a toolset to generate beautiful documents using modern web technologies. That would allow to switch internal components any time and use whatever is best. Right now asciidoctor.js is (obviously) the best choice of course :-)

wh81752 commented 3 years ago

Nevermind, the definition order is important, I need to disable the default --version and --help option and then declare them:

Yes, but why is the order important? In order to support "--help syntax" ??

How about dropping "--help syntax" and adding "--help-syntax" instead?

ggrossetie commented 3 years ago

I'm sure you disagree on this, nonetheless: My general approach would be that asciidoctor-web-pdf is a toolset just using asciidoctor (as library) rather than deriving from asciidoctor.

What do you mean by "deriving" from Asciidoctor? We are extending the Asciidoctor CLI because it's easier to maintain since the asciidoctor-web-pdf CLI is really just a wrapper around the asciidoctor CLI.

In theory, we could build an agnostic tool but it won't be useful until we have an AsciiDoc specification with a well-defined API (and when another JavaScript implementation is available).

So for now we are rely on Asciidoctor.js, Paged.js and Puppeteer but maybe in the future we will use other technologies/libraries.

Yes, but why is the order important? In order to support "--help syntax" ??

Because yargs treats help and version options with a custom logic. So when we are calling help(false) with disable the feature completely. So we need to disable the feature and then reenable it using a custom option.

How about dropping "--help syntax" and adding "--help-syntax" instead?

I don't want to do that because I want to keep the asciidoctor (JavaScript) CLI and the asciidoctor (Ruby) CLI consistent.

It's also the reason why asciidoctor-web-pdf and asciidoctor-revealjs are built-in on top of asciidoctor. It provides a common foundation and "feature parity" between tools.

ggrossetie commented 3 years ago

Should be fixed in https://github.com/Mogztter/asciidoctor-web-pdf/pull/496

@wh81752 Could you please give it a try?

wh81752 commented 3 years ago

@Mogztter - looks pretty good to me.

 --help              print a help message
                          show this usage if TOPIC is not specified or recognized
                          show an overview of the AsciiDoc syntax if TOPIC is syntax                            [string]

Options --help and --version are working as expected. Displayed help or usage text contains --help and --version.

Had some trouble to understand the help message itself; looks like yarg has a peculiar way of outlining the possible arguments. I expected something like:

 --help [TOPIC]     print a help message
                               show this usage if TOPIC is not specified or recognized
                               show an overview of the AsciiDoc syntax if TOPIC is "syntax"                            [string]

or

 --help     print a help message
                 show this usage if TOPIC is not specified or recognized
                 show an overview of the AsciiDoc syntax if TOPIC is "syntax"                            [TOPIC]
ggrossetie commented 3 years ago

looks pretty good to me.

Good, thanks for taking the time to review.

Had some trouble to understand the help message itself; looks like yarg has a peculiar way of outlining the possible arguments. I expected something like:

I agree but as far as I know we cannot easily change it. Having said that, it seems reasonable so we can probably open a feature request at https://github.com/yargs/yargs

ggrossetie commented 3 years ago

Closing, it was fixed in https://github.com/Mogztter/asciidoctor-web-pdf/pull/496 Thanks for raising this issue @wh81752