stil4m / elm-syntax

Elm syntax in Elm
MIT License
94 stars 28 forks source link

Idea: Rename Expression.FunctionOrValue and/or split it #168

Open jfmengels opened 2 years ago

jfmengels commented 2 years ago

We currently have a Expression.FunctionOrValue node, and the name is both long and a bit confusing. It is also not completely true, because the node is not a function, but a reference to a function or to a variable/argument/... .

I propose to rename this node. I was going to suggest Reference but that is somewhat confusing still. ESLint uses the term Identifier which I think fits quite well.

Maybe we also want to split this into 2 different nodes: one for variables/argument (add) and one for type constructors (True, SomeRecordAlias). I have found cases where this was important to the rule and I had to differentiate between the two (which I did by checking whether the first character was capitalized).

Any opinions on this?

MartinSStewart commented 2 years ago

I agree FunctionOrValue isn't a great name. But one advantage it has over Identifier is that it's clear it's not talking about type definition identifiers. Maybe that's sufficiently obvious to someone who knows you can't reference a type in an expression but I think making it extra clear is a good thing.

I agree distinguishing between ordinary functions and type constructors would be nice. What about Function and TypeConstructor?

Edit: On second though I don't like those names either since they don't make it clear it's a reference. FunctionIdentifier and TypeConstructorIdentifier maybe? Though TypeConstructorIdentifier is kind of long.

jfmengels commented 2 years ago

It looks like in the TypeScript AST the A in type A = ... is also an Identifier node.

I don't think we need to cater too much to people who don't know Elm that well, especially as I think it's quite clear that Expression.Identifier would not refer to the identifier in a type declaration. Also, if they're using in a context like elm-review, it's unlikely that they will be expecting to find a type declaration identifier while visiting expressions.

I don't like having Function in the name because it doesn't always reference a function (a when a = 1 for instance), and that can be confusing. Also, a type constructor identifier can also be a function.

We could name references to a ValueIdentifier, but again, even functions are technically values (at least in my definition :thinking: ).

Another solution is to only have Identifier and have a function that indicates whether the name would be a "type identifier" or not (so that my rules don't have to re-implement that logic in a way that could be flawed compared to the parser). I'm not sure where this function would go and how we would prevent it from running on strings that don't know from the Identifier node though.

lue-bird commented 2 years ago

Throwing in one more idea: Expression.Variable for values and functions

Splitting off a new variant for record type alias and type variant constructors might be overkill since many will mostly match both variables and constructors as one case.