stil4m / elm-syntax

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

V8: Use NonEmptyList #192

Open jfmengels opened 10 months ago

jfmengels commented 10 months ago

In the V8 branch (https://github.com/stil4m/elm-syntax/pull/188), a number of fields and custom type variant data have switched to create a guarantee that these things are never an empty list (ex: a record update without any field assignments).

The way it is done is inconsistent though:

The idea of making sure we can't have an empty list is great, but this should be consistent.

Proposal

My proposal is to use a NonEmptyList defined as type alias NonEmpty a = ( a, List a ). I'm not entirely sure where to define it, probably in a separate moduleElm.Syntax.NonEmptyList that only contains that definition. In practice people can then use a definition from other places if they like to such as turboMaCk/non-empty-list-alias. This would have several benefits.

  1. If we apply this everywhere, things would be more consistent.
  2. The breaking change would be smaller. We'd have Container (List X) switch to Container (NonEmptyList X) and Container { things : List X } switch to Container { things : NonEmptyList X }, which is not a large change in terms of API. Maybe it's a bit of a moot point but still.
  3. We would be grouping the first element and the second in a value that can be passed around easily. I imagine that we will often have functions that take the whole list of arguments for instance. With the current v8 API for Lambda, we'd probably often recreate the nonempty structure in the function call to keep the guarantee: Lambda { firstArg, restOfArgs } -> fn ( firstArg, restOfArgs ). With this approach, we could keep the call just like today, we'd only need to have the function take a NonEmptyList instead.
  4. There'd be less of a need to guess the name of the different fields. Is it firstArg or firstArgument?
  5. Users could use https://package.elm-lang.org/packages/turboMaCk/non-empty-list-alias/latest/ and other functions that work with tuples as non empty list, without having this package depend on a specific implementation of NonEmptyList (so less needed dependencies)

What do you think? Should we migrate all of our variants to use this NonEmptyList? Can you think of counter arguments?

jfmengels commented 10 months ago

I guess this is a follow up and/or reproposal of https://github.com/stil4m/elm-syntax/issues/43

MartinSStewart commented 10 months ago

This seems reasonable to me. Though, at the risk of bike-shedding, I prefer the name Nonempty over NonEmpty or NonEmptyList. It's short while being still clear in its meaning and doesn't require me to press the shift key in the middle of typing.

jfmengels commented 10 months ago

I'm willing to be convinced, and I agree with the pain of needing to press shift. But I wonder what the "grammatically" correct name is, and the library I linked to uses NonEmpty as the convention, so I'm not sure :/

For Nonempty vs NonemptyList, I guess it makes sense, since we're unlikely to have a NonemptySet in this repo.

lue-bird commented 10 months ago

If turboMaCk/non-empty-list-alias didn't already use NonEmpty for the same type, I'd prefer FilledList or ListFilled

jfmengels commented 10 months ago

Filled in my mind means that it's full, that there's no more space to add something. "Non empty" fits better in my opinion, and it's been used by a bunch of other people so it's less surprising.

An argument in favor of NonEmptyList is that it might not be immediately obvious in the docs what it is, if you see NonEmpty Case for instance. It doesn't help that the packages website won't turn NonEmptyList into a link to the type alias (it only does so for custom types if I'm not mistaken).

MartinSStewart commented 10 months ago

Okay this is too much bikeshedding for me. I'm fine with NonEmptyList!

miniBill commented 10 months ago

Strong approve of this. Tupley nonempty lists are also better for pattern matching!

Expression.Case {...} can only be pattern matches shallowly, Expression.Case (Node Expression) (NonEmptyList (Node Expression)) can be pattern matched deeply