Pretty printer library switch with ANSI compatibility

[newhoggy]

This PR modifiers the pretty printer to improve the structure of the output for readability.

Included in the change is the switch from ansi-wl-pprint to prettyprinter. The switch allows for the use of annotations, which allows for tracing the structure of the document for ease of debugging.

I'm looking for interest in merging improved formatting and switching to prettyprinter.

[newhoggy]

Before:

Usage: cardano-cli transaction build-raw [--byron-era | --shelley-era |
                                           --allegra-era | --mary-era |
                                           --alonzo-era] (--tx-in TX-IN
                                         [--tx-in-script-file FILE
                                           [
                                             (--tx-in-datum-file FILE |
                                               --tx-in-datum-value JSON VALUE)
                                             (--tx-in-redeemer-file FILE |
                                               --tx-in-redeemer-value JSON VALUE)
                                             --tx-in-execution-units (INT, INT)]])
                                         [--tx-in-collateral TX-IN]
                                         [--tx-out ADDRESS VALUE
                                           [--tx-out-datum-hash HASH]]
                                         [--mint VALUE (--mint-script-file FILE
                                           [
                                             (--mint-redeemer-file FILE |
                                               --mint-redeemer-value JSON VALUE)
                                             --mint-execution-units (INT, INT)])]
                                         [--invalid-before SLOT]
                                         [--invalid-hereafter SLOT]
                                         [--fee LOVELACE]
                                         [--certificate-file CERTIFICATEFILE
                                           [--certificate-script-file FILE
                                             [
                                               (--certificate-redeemer-file FILE |

                                                 --certificate-redeemer-value JSON VALUE)
                                               --certificate-execution-units (INT, INT)]]]
                                         [--withdrawal WITHDRAWAL
                                           [--withdrawal-script-file FILE
                                             [
                                               (--withdrawal-redeemer-file FILE |

                                                 --withdrawal-redeemer-value JSON VALUE)
                                               --withdrawal-execution-units (INT, INT)]]]
                                         [--json-metadata-no-schema |
                                           --json-metadata-detailed-schema]
                                         [--auxiliary-script-file FILE]
                                         [--metadata-json-file FILE |
                                           --metadata-cbor-file FILE]
                                         [--genesis FILE |
                                           --protocol-params-file FILE]
                                         [--update-proposal-file FILE]
                                         --out-file FILE

After:

Usage: cardano-cli transaction build-raw  
            [ --byron-era
            | --shelley-era
            | --allegra-era
            | --mary-era
            | --alonzo-era
            ]
            ( --tx-in TX-IN
              [ --tx-in-script-file FILE
                [ (--tx-in-datum-file FILE | --tx-in-datum-value JSON VALUE)
                  ( --tx-in-redeemer-file FILE
                  | --tx-in-redeemer-value JSON VALUE
                  )
                  --tx-in-execution-units (INT, INT)
                ]
              ]
            )
            [--tx-in-collateral TX-IN]
            [--tx-out ADDRESS VALUE [--tx-out-datum-hash HASH]]
            [ --mint VALUE
              ( --mint-script-file FILE
                [ (--mint-redeemer-file FILE | --mint-redeemer-value JSON VALUE)
                  --mint-execution-units (INT, INT)
                ]
              )
            ]
            [--invalid-before SLOT]
            [--invalid-hereafter SLOT]
            [--fee LOVELACE]
            [ --certificate-file CERTIFICATEFILE
              [ --certificate-script-file FILE
                [ ( --certificate-redeemer-file FILE
                  | --certificate-redeemer-value JSON VALUE
                  )
                  --certificate-execution-units (INT, INT)
                ]
              ]
            ]
            [ --withdrawal WITHDRAWAL
              [ --withdrawal-script-file FILE
                [ ( --withdrawal-redeemer-file FILE
                  | --withdrawal-redeemer-value JSON VALUE
                  )
                  --withdrawal-execution-units (INT, INT)
                ]
              ]
            ]
            [--json-metadata-no-schema | --json-metadata-detailed-schema]
            [--auxiliary-script-file FILE]
            [--metadata-json-file FILE | --metadata-cbor-file FILE]
            [--genesis FILE | --protocol-params-file FILE]
            [--update-proposal-file FILE]
            --out-file FILE

[newhoggy]

Annotations allow me to output HTML to describe the structure of the document that is being rendered to better understand layout bugs:

[newhoggy]

The idea behind the new style is it prefers compact display such as this:

[--json-metadata-no-schema | --json-metadata-detailed-schema]

But that if the length of the group should exceed the remaining space on the line, it should use structured multiline indented format:

[ ( --certificate-redeemer-file FILE
  | --certificate-redeemer-value JSON VALUE
  )
  --certificate-execution-units (INT, INT)
]

Or combinations where it makes sense:

[ (--mint-redeemer-file FILE | --mint-redeemer-value JSON VALUE)
  --mint-execution-units (INT, INT)
]

[newhoggy]

I created another version of this PR without the tracing annotations so it's clearer where the substantive differences are.

#429

[newhoggy]

Mentioning #273 and @quchen because the PR involves switching to prettyprinter.

[Bodigrim]

Switching a pretty-printing library means that every downstream package has to adapt, because Doc is prominently used in optparse-applicative API. To be honest, incurring such toll on 800+ packages just to ease your debugging process does not seem reasonable to me.

My understanding is that you have debugged the issue at hand. Is it possible to revert back to ansi-wl-pprint?

(I’m not a maintainer)

[newhoggy]

I had a go yesterday, but it seems ansi-wl-pprint has different layout rules for its combinators and I wasn't successful.

I pointed out tracing mainly to show that annotations are actually really convenient and could be used to annotate documents for richer output.

I had a go at haskell/criterion#241 to show that in this case the necessary changes aren't too bad.

To ease compatibility issues, we could minimise breaking changes. optparse-applicative could provide all the exports necessary to deal with documents.

For the criterion case because they import ansi-wl-pprint directly, they would still need to modify to not do that, but the change would be minimal.

We could have an interim release where we make changes to optparse-applicative to export the the symbols that libraries like criterion need so they don't have to import ansi-wl-pprint anymore before finally merging in the library switch.

[Bodigrim]

I pointed out tracing mainly to show that annotations are actually really convenient and could be used to annotate documents for richer output.

What specifically do annotations buy us in case of optparse-applicative in production setting? What kind of richer console output are we looking for?

I had a go at haskell/criterion#241 to show that in this case the necessary changes aren't too bad.

Yes, indeed, it is usually possible to migrate between pretty-printing libraries. But nevertheless it means that downstream dependencies must make necessary amends and releases, and pre-existing packages must be revised with optparse-applicative < 0.17. This is quite a sizeable amount of work for everyone.

To ease compatibility issues, we could minimise breaking changes. optparse-applicative could provide all the exports necessary to deal with documents.

I think that more re-exports of this sort could pose even bigger compatibility issues in a long term. Imagine that some combinator is changed, deprecated or removed from prettyprinter. How would a given release of optparse-applicative maintain a stable interface?

[lehins]

@Bodigrim It seems that ansi-wl-pprint is no longer maintained. It is deprecated on hackage (see ekmett/ansi-wl-pprint#26) in favor of prettyprinter

I think it only makes sense for the community to start switching to the prettyprinter everywhere. Naturally it would be best to minimize breakage, but I am sure it will not be possible to completely avoid it.

[HuwCampbell]

Thanks for the PRs John. It's always great to have people put time into optparse-applicative.

There are a couple of orthogonal points here, @Bodigrim's opinion is well warranted in my view. It's very important to me that optparse is API stable as a lot of libraries and programs depend on it both in the open source world and I imagine commercially as well (breaking peoples' builds for their choice of CLI library is a bit rich).

So the questions are:

• Is changing the underlying pretty printing library going to improve library sufficiently to warrant a reasonably large breaking change?
• Do we like some or all of these adjustments to the help text?

After the original issue mentioning the prettyprinter library, I did also make a branch to see what it would be like, but overall, my impression was that it's probably not worth the switch. ansi-wl-pprint is well maintained and stable, and while annotations are a nice feature, it's a bit of a can of worms as to what and how we might think to annotate, whether the annotations API should be user extensible and exposed, and so on.

It would probably make a coloured renderer a bit easier and more declaritive if that was a direction we decided to take, but in reality if we wanted coloured output we could do that already.

With regards to the actual rendering changes: I'm quite sure we should rethink how we do the hang when the program name and subcommand names are long as squashing the text into the side really does make it ugly. Starting them fresh on a new line with an indent really would be an improvement. I've been considering how it can be done without expanding the number of preference modifiers (again).

The pretty printing is a bit of a balancing act. I do find what you've got more readable above, but it also seems a bit verbose – one of your examples above requires 5 lines for only 3 options.

I'll run through the changes in detail when I get some more time.

Edit: apparently it's not well maintained any more.

[newhoggy]

What specifically do annotations buy us in case of optparse-applicative in production setting? What kind of richer console output are we looking for?

I was thinking perhaps HTML help or man page support would be nice. It's not clear that annotations are strictly necessary to do this, but I'm thinking in these cases we could use annotations to delay some of the rendering to a later phase so we can re-use more code.

[newhoggy]

With regards to the actual rendering changes: I'm quite sure we should rethink how we do the hang when the program name and subcommand names are long as squashing the text into the side really does make it ugly. Starting them fresh on a new line with an indent really would be an improvement. I've been considering how it can be done without expanding the number of preference modifiers (again).

The pretty printing is a bit of a balancing act. I do find what you've got more readable above, but it also seems a bit verbose – one of your examples above requires 5 lines for only 3 options.

I'm wondering if a good compromise would be to have instead of this:

[ ( --certificate-redeemer-file FILE
  | --certificate-redeemer-value JSON VALUE
  )
  --certificate-execution-units (INT, INT)
]

something like this:

[ (--certificate-redeemer-file FILE | --certificate-redeemer-value JSON VALUE)
  --certificate-execution-units (INT, INT)
]

The rule would be if the list of options has no children, then the list will be rendered in compact form. In the above (--certificate-redeemer-file FILE | --certificate-redeemer-value JSON VALUE) would wrap, so the entire fragment would actually be four lines rather than the three that I've shown.

I'm guessing that it might actually end up like this:

[ (--certificate-redeemer-file FILE | --certificate-redeemer-value
  JSON VALUE)
  --certificate-execution-units (INT, INT)
]

[newhoggy]

Another perhaps out-there idea would be to use rainbow parens 😁 .

[Bodigrim]

@lehins thanks, I did not know that ansi-wl-pprint is deprecated, this is a change of perspective indeed. Nevertheless it still works fine at least with current GHC HEAD (future 9.4), so there is plenty of time to figure out migration plan.

[newhoggy]

I'm cool for this to take a long as it needs, since we're currently on a fork.

My hope is for the switch to happen at an appropriate and opportune time and to either improve the layout in the library or otherwise export some way for users to have more control of the layout externally.

[Bodigrim]

Out of curiosity I checked this branch against a standard tasty test suite.

Before:

Usage: prop-compiled [-p|--pattern PATTERN] [-t|--timeout DURATION]
                     [-l|--list-tests] [-j|--num-threads NUMBER] [-q|--quiet]
                     [--hide-successes] [--color never|always|auto]
                     [--ansi-tricks ARG] [--quickcheck-tests NUMBER]
                     [--quickcheck-replay SEED] [--quickcheck-show-replay]
                     [--quickcheck-max-size NUMBER]
                     [--quickcheck-max-ratio NUMBER] [--quickcheck-verbose]
                     [--quickcheck-shrinks NUMBER]

After:

Usage: prop-compiled
                       [-p|--pattern PATTERN]
                       [-t|--timeout DURATION]
                       [-l|--list-tests]
                       [-j|--num-threads NUMBER]
                       [-q|--quiet]
                       [--hide-successes]
                       [--color never|always|auto]
                       [--ansi-tricks ARG]
                       [--quickcheck-tests NUMBER]
                       [--quickcheck-replay SEED]
                       [--quickcheck-show-replay]
                       [--quickcheck-max-size NUMBER]
                       [--quickcheck-max-ratio NUMBER]
                       [--quickcheck-verbose]
                       [--quickcheck-shrinks NUMBER]

It's probably a matter of habit, but to be honest I like previous pretty-printing better. That's because I usually have a half-screen tall console window, so vertical space is quite precious. Ideally I'd prefer something even more condensed than currently, e. g.,

Usage: prop-compiled 
    [-p|--pattern PATTERN] [-t|--timeout DURATION] [-l|--list-tests] 
    [-j|--num-threads NUMBER] [-q|--quiet] [--hide-successes] 
    [--color never|always|auto] [--ansi-tricks ARG] [--quickcheck-tests NUMBER] 
    [--quickcheck-replay SEED] [--quickcheck-show-replay] 
    [--quickcheck-max-size NUMBER] [--quickcheck-max-ratio NUMBER] 
    [--quickcheck-verbose] [--quickcheck-shrinks NUMBER]

Maybe a choice of layout could be configurable to a certain extent?..

[newhoggy]

Thanks @Bodigrim for trying it out.

Some configurability would be nice.

There's also the possibility of choosing different layouts based on heuristics. For example if the parser is highly nested, it starts to get confusing so a structured layout is preferable, whereas if the parser is mostly linear, then compact is usually better.

The came CLI tool may have the same multiple commands with different levels of complexity so a heuristic based layout selector would actually be quite nice.

[lehins]

@Bodigrim I personally find this horrendous:

Usage: prop-compiled 
    [-p|--pattern PATTERN] [-t|--timeout DURATION] [-l|--list-tests] 
    [-j|--num-threads NUMBER] [-q|--quiet] [--hide-successes] 
    [--color never|always|auto] [--ansi-tricks ARG] [--quickcheck-tests NUMBER] 
    [--quickcheck-replay SEED] [--quickcheck-show-replay] 
    [--quickcheck-max-size NUMBER] [--quickcheck-max-ratio NUMBER] 
    [--quickcheck-verbose] [--quickcheck-shrinks NUMBER]

While this I personally find very eye appealing and much more readable as well:

Usage: prop-compiled
                       [-p|--pattern PATTERN]
                       [-t|--timeout DURATION]
                       [-l|--list-tests]
                       [-j|--num-threads NUMBER]
                       [-q|--quiet]
                       [--hide-successes]
                       [--color never|always|auto]
                       [--ansi-tricks ARG]
                       [--quickcheck-tests NUMBER]
                       [--quickcheck-replay SEED]
                       [--quickcheck-show-replay]
                       [--quickcheck-max-size NUMBER]
                       [--quickcheck-max-ratio NUMBER]
                       [--quickcheck-verbose]
                       [--quickcheck-shrinks NUMBER]

So this is quite a subjective matter and I do agree that an option to configure would be awesome! In fact, if that could be a user supplied option this would be even better ;)

That's because I usually have a half-screen tall console window, so vertical space is quite precious"

I find it hard to believe that this is a widespread setup, because monitors are all widescreen nowadays and splitting vertically is more efficient. That's why 80 char wrapping is still prevalent while coding

[newhoggy]

I was considering, whether with prettyprinter annotations we could sprinkle some layout hint annotations in the document and postpone final layout decisions to the end where it is much easier to configure.

We could even do a rainbow parens thing at the final stage of converting a docstream to text for those who want compact representation of highly nested options to still be somewhat readable.

[HuwCampbell]

Thanks all for the input.

There's no best way to pretty print options and arguments in all situations for all programs, and I think this thread is a bit too opinionated. It's not worth the stress.

With regards to this PR: it's too big for me to merge as is, it changes the underlying dependency tree as well as changing the rendering in ways which are at the very least controversial.

Here's my suggestion:

• We look at my overflow branch to fix the immediate issue or the options being too squished up when there are long command names;
• We refocus this PR to just the pretty printer change and not change the actual look; and
• We raise an issue (not a PR) to discuss other changes to pretty printing.

That sound cool?

Huw

[newhoggy]

Your suggestion is perfect. I have been working on ANSI colour support, which will be necessary to do the switch because optparse-applicative exports such functions. I can submit a PR to include both the library switch and the ANSI work.

[newhoggy]

I've removed all the code that is unrelated to the switch to prettyprinter.

[andreasabel]

I second the switch to prettyprinter.
I think the breakage caused by this is mitigated by the fact that optparse-applicative exports Doc and its combinators.

[Bodigrim]

Looks good from my perspective, I can still run bytestring and text test suite without cyclical dependency.

[Jimbo4350]

Is this PR good to be merged? Is there anything else that needs to be done? @HuwCampbell @andreasabel @Bodigrim

[andreasabel]

CI hasn't run yet. @HuwCampbell , could you please hit the button?

[HuwCampbell]

I've set this to run in CI.

So my only real issue is that this doesn't really do that much. The focus here was to be as backwards compatible as possible which is good, but it's inlining a few functions which aren't exported, and really only sticking to what we can do at the moment.

I have a little branch I'm working on as well (indeed I've had a look a few times), but I think if we want to make a breaking change it should offer more real advantages.

My current thought is that we should probably use the annotations for a bit more than what is currently available, for example, optionally making the | in alternatives duller when desired.

I know it's not super satisfying, but I don't want to push two big breaking changes to the pretty-printer.

I'll set some time in my calendar for September to finish it off and figure out a nice API for user preference.

[johnk-da]

I pushed a fix for older compilers:

• Removed use of TypeApplications
• Import Data.Semigroup

[newhoggy]

Any progress on moving to prettyprinter?

[CarlosLopezDeLara]

Any updates on this one? Are there still any concerns? Thanks

[HuwCampbell]

Thank you for putting in this work, I used a bunch of the ideas in a recent release which achieves these goals.

[newhoggy]

Thanks @HuwCampbell for putting together the changes for the new release.