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

Consider using prettyprinter instead of ansi-wl-pprint #273

Closed
quchen opened this issue Jul 3, 2017 · 17 comments
Closed

Consider using prettyprinter instead of ansi-wl-pprint #273

quchen opened this issue Jul 3, 2017 · 17 comments

Comments

@quchen
Copy link

quchen commented Jul 3, 2017

Hi there,

tl;dr: I was wondering whether you would be open to adapting the prettyprinter for optparse-applicative (instead of ansi-wl-pprint).


Longer, want to read

I recently released a vastly overhauled version of our beloved Wadler/Leijen prettyprinter, called prettyprinter.

I believe many people were unhappy with certain aspects of wl-pprint, and many of them felt like adding »that one thing«, which left us with a plethora of Wadler/Leijen prettyprinters, each one with one shortcoming less, but none of them lacking most of the other features. prettyprinter was designed to remedy this, by combining all the popular features (as demanded by multiple popular projects, including e.g. GHC, Idris, Trifecta, and yes, optparse-applicative), and adding excellent documentation and migration advice ontop of it to make the process as simple as possible.

Feedback to prettyprinter has been very positive on release, and it is even under consideration for use internally in GHC.

What’s to be gained?

Why I’m asking here? Well, optparse-applicative is a very popular library, and I see it as one of the roots of ansi-wl-pprint usage in the Haskell ecosystem. I think there is much to be gained (in terms of pain avoided) by having a standard library for prettyprinting that does all the things and does it well; users should not have to have to use an ANSI terminal dependency without the capability of annotations when they also want good command line parameter handling.

Having optparse-applicative switch to using prettyprinter would be a huge milestone to unification of all the prettyprinters, ideally deprecating the others in its favor, should it be able to gain enough traction.

What’s to be lost?

There are conversion functions from Doc (ansi-wl-pprint) to Doc (prettyprinter) and back, so users that really want/have to work with ansi-wl-pprint can still use it. Apart from this, I don’t expect any problems worth mentioning.

The conversion functions are found in the not yet released prettyprinter-convert-ansi-wl-pprint package, which will be released as soon as ansi-wl-pprint makes its next Hackage release (which I’m working on these days).

Why this prettyprinter, and not one of the others?

This is what prettyprinter’s readme is all about: README.md

I’d be very happy if we could put the Wadler/Leijen dilemma behind us by providing a good single choice for everyone, and I think optparse-applicative could be a great help achieving this.

…the end :-)

@HuwCampbell
Copy link
Collaborator

Thanks for writing. I will consider this it. I'm not planning any major releases any time soon however.

@quchen
Copy link
Author

quchen commented Jul 5, 2017

Good to hear!

The nice thing is that this might not require a major version bump as soon as the conversion module is released :-)

@HuwCampbell
Copy link
Collaborator

No it would still require a major release.

We export the symbols from ansi-wl-pprint and require its Doc type in our builder API. Exporting replacement symbols with the same names is a breaking change, as anyone who uses the Doc types directly will be required to change their dependencies or face a type error.

Criterion for instance does this, and they would need to change their dependencies and limit their versions of optparse-applicative to the next major version and above.

This is the main reason why it's not an easy call, as you've said, optparse-applicative is a popular package, and breaking changes have the potential to inconvenience a lot of people.

@quchen
Copy link
Author

quchen commented Jul 6, 2017

It would require a major release if the API introduced breaking changes. But that’s not necessarily the case – if the internals are switched, but the public API stays the same, we would not need a major version bump. In other words: change the internals, but add conversion to the old Doc type at the outermost layer so versions stay compatible. Then add new functions, mimicing the old ansi-wl-pprint based ones.

The downside of this approach is that during the transitional phase, we couldn’t drop the (transitive) dependency of ansi-wl-pprint, and the API involving some Doc would have to be there twice, once old, once new (with the »old« just being thin wrappers around the »new« to convert back to ansi-wl-pprint formats).

@ad-si
Copy link

ad-si commented Jul 3, 2018

Strong plus one from me. What are the next steps on this issue?

@HuwCampbell
Copy link
Collaborator

Sorry I do have some reticence, but this does seem reasonable to pursue for the next major version of optparse-applicative.

So why am I hesitant? a few reasons:

For one, as optparse is pretty much at the bottom of a lot of programs dependency graphs, I need to be really careful about what I dependencies I add and change (for example, I haven't even added a dependency on text);

ansi-wl-pprint is really, really stable, which is good, as I don't have to think about it shifting underneath me and chasing dependencies (which effects users as well);

optparse is extremely backwards compatible, with support back to ghc 7.0, and it probably still works for hugs as well. This isn't the biggest deal, but it looks like prettyprinter's oldest supported is 7.8;

Lastly, I think our current output is already quite pretty, and I don't think that optparse's choice of pretty printer really effects anyone else's. Option parsing is mostly self contained, and in the worst case, one can call toAnsiWlPprint if they need to.

Those points said: prettyprinter seems pretty good which regard to its dependencies and stability, the only things which would be added are text and void, which I think is very reasonable.

So what needs to actually happen to push this forwards? I need to look at how this will change the API and potentially break code and peoples dependency graphs. As there are pretty reasonable conversion tools for the current printing library, I could, for instance, leave the current builder types as they are,

-- | The type stays the same, a deprecation annotation may be added at some point in the future.
headerDoc :: Maybe Ansi.Doc -> InfoMod a
headerDoc doc = InfoMod $ \i -> i { infoHeader = Chunk (fromAnsiWlPprint doc) }

I could also then, potentially, add new, mostly equivalent functions:

-- | This is new. I'll get rid of the `Maybe`... it's a bit annoying.
headerPretty :: Pretty.Doc -> InfoMod a
headerPretty doc = InfoMod $ \i -> i { infoHeader = Chunk (Just doc) }

@ad-si
Copy link

ad-si commented Jul 9, 2018

Sorry I do have some reticence, but this does seem reasonable to pursue for the next major version of optparse-applicative.

So let's build the next major version 😜

I think our current output is already quite pretty

One word: Colors
CLI tools without color output is an absolute no go for me nowadays.
Currently I'm using ansi-wl-pprint to build up a template, render it and replace all the placeholder variables with the colored version of prettyprinter. It's really ugly.

leave the current builder types as they are
I could also then, potentially, add new, mostly equivalent functions:

Sounds like a good plan! However, I was also thinking maybe we should just release a complete new package optparse-pretty. This would mean a lot less work regarding checking if it breaks code.

@HuwCampbell
Copy link
Collaborator

Sounds like a good plan! However, I was also thinking maybe we should just release a complete new package optparse-pretty. This would mean a lot less word regarding checking if it breaks code.

Nah. I don't think that's a good idea.

@phadej
Copy link
Contributor

phadej commented May 6, 2019

@HuwCampbell about old GHCs, see quchen/prettyprinter#74

We can try to convince @quchen to allow extending support back to GHC-7.0, if it would make you more happy to switch to prettyprinter.


As there are pretty reasonable conversion tools for the current printing library, I could, for instance, leave the current builder types as they are,

I'd advice against. Better to push conversions to the downstream. If they cannot move to prettyprinter, they could depend on conversion packages.

Otherwise optparse-applicative would depend on two prettyprinter libraries, instead of one: that's not good.

To emphasize, IMO optparse-applicative doesn't need a transitional phase, people can use old optparse-applicative until they have time to migrate.


Some packages do use prettyprinter for their own needs, e.g. dhall. Or don't use prettyprinting stuff in optparse-applicative (I think I never used). So the migration will be either welcomed (yay, no ansi-wl-pprint dependency) or easy (it compiles, yay!).

@quchen
Copy link
Author

quchen commented May 20, 2019

I’m open to making prettyprinter compatible back to 7.0 if it helps adoption!

@mitchellwrosen
Copy link

Bump, I'm still interested in this :)

Any blockers for this moving forward @HuwCampbell?

@newhoggy
Copy link

newhoggy commented Jul 14, 2021

Also interested in this. Is anyone working on this on optparse-applicative itself?

@cdornan
Copy link

cdornan commented Oct 28, 2022

@HuwCampbell I am in favour of the switch to prettyprinter because that seems to be the direction of travel for the wider ecosystem — I am basically converting from prettyprinter for optparse applicative.

All said, however, I love the care and attention you are taking on this and everything, your wilingness to engage and your emphasis on stability. There is no urgency. A model for the rest of us.

@mpilgrem
Copy link

Since November 2020, the ansi-wl-pprint package has been marked as deprecated on Hackage (ekmett/ansi-wl-pprint#18 (comment)), in favour of prettyprinter. With the release of ansi-terminal-1.0, I sought to relax the upper bound of ansi-wl-pprint's dependency on ansi-terminal (ekmett/ansi-wl-pprint#29). (My original motivation was that Stack depends on optparse-applicative, optparse-applicative depends on ansi-wl-pprint, and I wanted to avoid allow-newer-deps: ansi-wl-pprint in Stack's stack.yaml. However, it seems that this may also be a block to getting ansi-terminal-1.0 in Stackage snapshots.)

At ansi-wl-pprint, I've been pointed in the direction of this issue. However, I understand from what I've read here that the maintainers of optparse-applicative do not wish to end its dependency on ansi-wl-pprint because:

  1. both optparse-applicative and ansi-wl-pprint are mature and stable packages; and
  2. the Text.PrettyPrint.ANSI.Leijen.Doc type is used directly by some of the many users of the optparse-applicative package and that type differs from the Prettyprinter.Doc type.

Coming to this cold, it reads to me like there may be an impasse. Is there a way forward?

@HuwCampbell
Copy link
Collaborator

Hi. I'm sorry that this looks to be causing a real issue.

Yes, when this first came about I didn't see any pressing need to switch over because the current library was serving us well and I didn't want to push a breaking change onto people.

There have been a few PRs which have sought to make the swap, but it wasn't clear to me that anything would really be gained. As ansi-wl-pprint is 2 modules, the compilation time and dependency burden there seems particularly minor.

One thing which PRs made clear was that exactly what to put into annotations seemed was a bit inconclusive. We could keep just the ansi codes and be as close to ansi-wl-pprint as possible, but that seems a bit dissapointing, and a more semantically meaningul annotation would be more appropriate. Adding a type parameter though just seemed way over the top.

I can definitely put up a PR to move this forwards with a minor set of annotations. I'll try to do so this week. But I believe that if ansi-wl-pprint still compiles properly with ansi-terminal, then hackage maintainers could relax the bounds there as well too to unblock things.

@HuwCampbell
Copy link
Collaborator

Just to keep people updated.

The most recent version of ansi-wl-pprint reëxports prettyprinter under a small compatibility layer.

I've just put together a patch branch for 0.17.1 which widens the bounds to include that version. This involved rewriting some of our Pretty module as we were using some internals to get some subtle layout rules working.

I'll release that in the coming days. After that, the next version major version, 0.18.0, we'll shift the dependency across.

@HuwCampbell
Copy link
Collaborator

I've just release two new versions:

0.17.1

Can use prettyprinter via a compatibility layer

0.18.0

Uses it directly.

Thanks for the input all.

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

No branches or pull requests

8 participants