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

use a state monad for keeping track of the output state #25

Closed
cdepillabout opened this issue Mar 8, 2018 · 12 comments · Fixed by #67
Closed

use a state monad for keeping track of the output state #25

cdepillabout opened this issue Mar 8, 2018 · 12 comments · Fixed by #67

Comments

@cdepillabout
Copy link
Owner

I talked a little about this in my response in #24.

This would look similar to how it is done in hindent.

@sureyeaah
Copy link
Contributor

Hey, I would like to work on this. Any particular things I should keep in mind?

@cdepillabout
Copy link
Owner Author

@sureyeaah Thanks for taking a look at this!

Any particular things I should keep in mind?

I'd take a glance at the hindent source code and see how it uses StateT (in its Printer newtype). I think a similar thing could be done for the pretty-simple output code.

Also, the printing code for pretty-simple is pretty hacky, so feel free to make big changes.

There are lots(?) of tests for pretty-simple, so you should feel confident in making changes.

@cdepillabout
Copy link
Owner Author

cdepillabout commented Dec 3, 2019

@sureyeaah Oh, I did think of one more thing you should keep in mind.

pretty-simple is able to parse and pretty-print lazy data structures, so it is important that this property is kept.

When you are working on this, just make sure that pretty-printing lazy data structures (like [1..]) still works.

@sureyeaah
Copy link
Contributor

The ExprToOutput module uses a state monad already. Where exactly is it that we need to make changes then?

@cdepillabout
Copy link
Owner Author

@sureyeaah The Text.Pretty.Simple.Internal.OutputPrinter module contains the render function, which turns a list of Output elements into Text.

It does this by writing to a Text.Builder directly. Instead, it would probably be easier to use a State monad, similar to the ExprToOutput module. This should also be similar to other Haskell pretty-printing libraries, like hindent.

@cdepillabout
Copy link
Owner Author

Another thing that might help simplify this is actually using a proper pretty-printing library:

https://hackage.haskell.org/package/prettyprinter

Although a big thing to keep in mind is whether this would be sufficiently lazy.

@sjakobi
Copy link
Contributor

sjakobi commented Jan 21, 2020

Another thing that might help simplify this is actually using a proper pretty-printing library:

https://hackage.haskell.org/package/prettyprinter

Oh, no! ;)

I actually rely on pretty-simple to debug prettyprinter. This would introduce a circular dependency!

(I could probably work around a circular dependency though…)

@sjakobi
Copy link
Contributor

sjakobi commented Jan 21, 2020

Note BTW that there's already

http://hackage.haskell.org/package/show-prettyprint

which is a bit like a pretty-simple built on top of prettyprinter. I think its output is less nice than pretty-simple's though.

@cdepillabout
Copy link
Owner Author

Ah, I see :-)

I guess keeping the dependencies of pretty-simple minimal would be a good idea, so maybe using a different pretty-printing library, or re-implementing a simple pretty-printing library in pretty-simple would make sense.

@georgefst
Copy link
Collaborator

@sjakobi out of interest, would you be able to get away with:

cab install pretty-simple -f buildexe

pp :: Show a => a -> IO ()
pp = putStrLn <=< readProcess "pretty-simple" [] . show

I've used this trick before to avoid actually depending on pretty-simple.

I think there's a reasonable argument for using an actual pretty-printing library, and prettyprinter itself has minimal dependencies.

@georgefst
Copy link
Collaborator

Anyway, @sureyeaah, did you make any progress here?

I figure it may simplify #66, as well as the existing #56 and #43.

@sjakobi
Copy link
Contributor

sjakobi commented Jun 4, 2020

@georgefst Thanks, that looks like a nice trick!

I think I can also recommend using prettyprinter at this stage. It's pretty polished and there are no plans to adopt any dependencies that aren't core libraries. There have even been some ideas to somewhat "standardize" its use, so libraries like ansi-wl-pprint could eventually be deprecated.

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

Successfully merging a pull request may close this issue.

4 participants