Add `--pretty-operators` flag - Githubissues

faylang / fay

A proper subset of Haskell that compiles to JavaScript

https://github.com/faylang/fay/wiki

BSD 3-Clause "New" or "Revised" License

1.29k stars 86 forks source link

Add `--pretty-operators` flag #425

Closed michalseweryn closed 9 years ago

michalseweryn commented 9 years ago

Solve issue #366, plus some improvements of Printer

bergmark commented 9 years ago

Please fix these warnings

src/Fay/Types/Printer.hs:42:1: Warning:
    Top-level binding with no type signature:
      pwOutputString :: PrintWriter -> String

src/Fay.hs:30:1: Warning:
    The import of ‘Fay.Compiler.Print’ is redundant
      except perhaps to import instances from ‘Fay.Compiler.Print’
    To import instances alone, use: import Fay.Compiler.Print()

bergmark commented 9 years ago

I'd like to have a test case that compiles something with both --pretty-operators and --pretty-thunks (I forgot to mention this in your --pretty-thunks PR) to make sure it works, and for regression testing. You can add this in tests/Compile/ to treat it specially and run it from the Test.Compile module.
This seems to contain breaking changes, such as PrintState no longer being exported from Fay.Types. If we can keep something from being a breaking change (such as doing a re-export) that's preferable, and I'd also like a list of what the breaking changes and non-breaking additions to the api are to put in the changelog.

Does this sound okay?

Thanks a lot for the PR, And special thanks for doing some cleanup, it's always hard to do that on external projects!

michalseweryn commented 9 years ago

Okay, I'll take a look at this today.

bergmark commented 9 years ago

Cheers!

michalseweryn commented 9 years ago

I was looking for some tests of --pretty flag, but I didn't find any. Maybe I should add some test of this flag as well?

bergmark commented 9 years ago

Sure, that'd be helpful as well!

bergmark commented 9 years ago

Another idea is to add a --pretty-all flag that enables all these options, would be convenient. I'm happy to merge this PR without that though!

michalseweryn commented 9 years ago

A about breaking changes, these are:

Export CompileResult from Fay.Types
Export PrintReader and PrintWriter, and reducing number of PrintState fields (these are moved to PrinterReader and PrintWriter)
In the meanwhile PrintWriter field type changed - output is now stored as ShowS - tests run faster now
Replace monadic functions (askP, getP, ...) with higher level ones: indented, askIf, newline, write, mapping. askIf is pretty much like previous askP, but works like if-then-else.

bergmark commented 9 years ago

This is great stuff!

Main.main = new $(function(){
  return _(_(Prelude["$"])(Prelude.print))(_(_(Prelude["++"])(Main.g))(Fay$$list("b")));
});

One thing I noticed is that it (understandably) doesn't prettify the operators built into the runtime, so you get Fay$$then, Fay$$add etc instead of e.g. Prelude[">>"]. It could probably be changed but I'm not sure if it's worth it since these functions are named already. Changing it would also mean that another layer of thunks would be added, which might make debugging harder.

Thank you so much!