stil4m / elm-syntax

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

Put all node types in the same module #196

Open jfmengels opened 10 months ago

jfmengels commented 10 months ago

elm-syntax has a LOT of modules to describe AST nodes such as Expression and Declaration. I count ~15, depending on what you want to count in that or not.

That is quite a lot and can cause a bit of annoyance when you need to add one more import because you're reaching for a new type.

A potential move would be to move all the types relating to node types into the same module, probably named Elm.Syntax.

That is the approach that @rupertlssmith's the-sett/elm-syntax-dsl has gone with, as well as @mdgriffith's elm-codegen (though it went for 3 different modules instead of one), as well as elm-format (and gren-format, the more up-to-date formatter). @avh4 told me this was a good approach (can't exactly remember the reasons he cited though).

import Elm.Syntax.Expression as Expression
import Elm.Syntax.Pattern as Pattern

visitExpression node =
   case Node.value expr of
     Expression.FunctionOrValue _ _ -> ...
     _ -> ...

visitDeclaration node =
   case Node.value expr of
     Declaration.FunctionDeclaration f -> ...
     _ -> ...

we would have something closer to this

import Elm.Syntax as Syntax

visitExpression node =
   case Node.value expr of
     Syntax.FunctionOrValue -> ...
     _ -> ...

visitDeclaration node =
   case Node.value expr of
     Syntax.FunctionDeclaration f -> ...
     _ -> ...

My biggest pain point with this proposal (and if we can find a satisfying solution then I think we should go ahead with this idea) is the naming and the naming conflicts.

There are a lot of naming conflicts because similar elements can appear in different contexts. For instance, there is Unit for Expression, for TypeAnnotation, for Pattern and for DestructurePattern (these last two have conflicts for pretty much all of them).

The obvious solution would be to prefix every constructor, which is what elm-format does.

That said, while it works really well for a tool like elm-format where those types are not "consumed" by users (they're just internal), this package has a lot of consumers, and having them understand the different between Syntax.Unit, Syntax.TUnit, Syntax.PUnit and Syntax.DPUnit is probably not the best experience.

We could have the prefix much longer, like PatternUnit but that does make everything much longer and detracts from a nice UX as well.

I'm open to ideas, opinions and suggestions on this.


Some types I don't know whether they should be in the same module or not:

lue-bird commented 10 months ago

Modules group types and operations nicely

e.g.

module Expression exposing (Expression(..), encode, decode, directChildren, map)
type Expression = Unit | Parenthesized Expression | ...

is pretty obvious and the documentation is more searchable compared to

module Syntax exposing (..., Expression(..), expressionEncode, expressionDecode, expressionDirectChildren, expressionMap)
type Expression = ExpressionUnit | ExpressionParenthesized Expression | ...

Might all the needed aliasing by the problem?

Many imports aren't the actual problem here I think (at the very least for those that have auto-import support).

The biggest manual changes you have to do are usually the aliasing (and that's usually also very little work compared to the lifetime of the file). If module names were shorter like in elm-codegen, say

import Elm.Expression
import Elm.Pattern

visitExpression node =
   case Node.value expr of
     Elm.Expression.FunctionOrValue _ _ -> ...
     _ -> ...

visitDeclaration node =
   case Node.value expr of
     Elm.Declaration.FunctionDeclaration f -> ...
     _ -> ...

We'd have an equally concise alternative to what you suggested that still groups nicely. We then just have to be careful not to create module name clashes with elm-codegen which seems possible. Alternatively, we could choose a different short module prefix like Elm.Ast, ElmAst or ElmCode. or just not use a separating dot: Elm.Pattern → ElmPattern

Though if you'd ask me, I think I'm actually already fine with the status quo here.