stil4m / elm-syntax

Elm syntax in Elm
MIT License
92 stars 26 forks source link

Get rid of Elm.Writer #198

Open jfmengels opened 10 months ago

jfmengels commented 10 months ago

I know some people use the Elm.Writer module, but in practice it is a pretty poorly done module, in the sense that we generally recommend people to use elm-syntax-dsl instead.

I propose that we get rid of this feature, and let this be handled by separate packages like elm-syntax-dsl.

MartinSStewart commented 10 months ago

We should definately get rid of the current Elm.Writer implementation. I would like to (with @rupertIssmith's permission) copy elm-syntax-dsl's writer into elm-syntax for the sake of convenience though.

jfmengels commented 10 months ago

Not sure you pinged @rupertlssmith correctly (if that was your intention), since his handle is not a link :thinking:

rupertlssmith commented 10 months ago

Its licensed permissively so you can copy it without my explicit permission, but thanks for asking anyway! I will let you decide what is the right way to go.

Matt Griffith already copied it into elm-codegen.

My only thought on copying is that if I later make improvements you won't automatically get them. On the other hand, you will remain stable and not be exposed to my 'improvements'.

The pretty printer never quite made to a 100% complete state. 100% complete would be where it can handle any and all Elm code and exactly match the format of elm-format. There were a few wrinkles that I never managed to completely work out. I did try and document them though: https://github.com/the-sett/elm-syntax-dsl/tree/master#broken-stuff-in-elm-syntax

You might end up fixing some of these wrinkles perhaps? I think most likely you will look at the code and think holy cr*p, because its really quite hard to get your head around. I was nearly going insane by the time I got it 98% of the way there...

The other improvement that I hope to make at some point would be to add color syntax highlighting ability. This is already possible with the the-sett/elm-pretty-printer - its just a question of going through and tagging all the parts of the Elm AST appropriately so that the tags can then be translated to colors during printing if you want the colors.

rupertlssmith commented 10 months ago

Does Elm.Writer manage to preserve single line comments?

That is also something I could not do, since there is no space in the AST to keep them. My pretty printer will just strip out any single line comments.

But comments are kept in the Elm.Syntax.File:

https://package.elm-lang.org/packages/stil4m/elm-syntax/latest/Elm-Syntax-File

I have a feeling that if you use elm-syntax to parse and then write a file, it does manage to preserve the comments, since they are List (Node Comment) and the Node has a Range giving the position in the file, even though it is not embedded in the AST?

32 - Looks like Elm.Writer does strip them too. So you won't lose anything special there by removing it.

rupertlssmith commented 10 months ago

Just another thought - if you do copy the pretty printer here, it means I can remove it from future versions of elm-syntax-dsl and then contribute to maintaining the version here. Probably similar story for elm-codegen. So from that perspective copying it here probably does work better for maintaining it.

jfmengels commented 10 months ago

I'm not against re-implementing Elm.Writer by copying elm-syntax-dsl's implementation. I don't know how much work I want to put in it, as it's not something I might use.

Having an in-Elm elm-format could be pretty cool for elm-review's fixes though, but as I understand it, even that implementation is pretty far from that still.

I really have no clue how much energy I will be able to muster into this project, but I'm not against either ideas of removing the module and forwarding people to elm-syntax-dsl or copying that implementation into Elm.Writer.

rupertlssmith commented 10 months ago

Rereading your OP, you just to remove the Writer and leave my pretty printer where it is. Also makes sense. Or I can split it out into its own package so you can have the printer without the codegen. Whoever does the work gets to decide!

Maybe we should ping folk on slack and ask if anyone actually wants this Writer. No objections from me if its deleted.