Allow most leading and trailing delimiters #4345
Conversation
This is why I initially brought up specifically ADTs to narrow the scope 😅 |
|
This PR can be broken down into smaller changes and their corresponding discussions. I'll provide an example of the change and name it: -- Optional leading pipe character for first data constructor
data Foo =
| Bar
| BazI think I'm good with making this change because
-- Optional trailing pipe character for last data constructor
data Foo =
Bar |
Baz |This change feels a bit weird to me because the pipe character is no longer in front of the data constructor. So, this change feels like a "bigger" one than the one above. Granted, I know that The only argument I can think of against this is that one could copy-paste the above constructors into a file and then replace As for module exports, below are a few examples of how this could be formatted: -- Current syntax. Uses 4 lines
module Module1
( (<>)
, b
) where
-- Optional leading comma character for first export
-- uses 5 lines
module Module2
(
, (<>)
, b
) where
-- Same as Module2 but formatted differently to use 4 lines
module Module3 (
, (<>)
, b
) where
-- Optional trailing comma character for last export
-- uses 4 lines
module Module4 (
(<>),
b,
) whereLet me briefly comment on these examples. The order of exports determines their order in the resulting documentation. That being said, I don't know if I care about the module export syntax. The only time someone touches export lists manually is when they want to prevent some things from being exported. Since this happens infrequently, I think that's an argument against making this change: this small and infrequent annoyance due to a syntax limitation is less of an issue than formatting wars. (Side note... While @rhendric added a test for As for module imports, I don't know if this issue matters that much either. Neither the order of the modules, nor their import lists matter, so long as they are imported. Moreover, the IDE can add imports via autocompletion, remove unused imports by applying the suggestions inferred after analyzing import-related compiler warnings, and sort them if one is already using a formatter like As for type class constraints in type class definitions and instances, things are a bit different than module import syntax. Constraint order does not matter, so no argument here due to the -- single line
class (Foo a) => Bar a
-- multi-line
instance
( Foo a
, Foo b
) => Foo (Tuple a b)So, I think the comment/uncomment workflow argument I made for module export syntax can apply here, but the ordering one and the infrequency arguments don't apply. Given this tradeoff, I think avoiding a formatter war is more important than removing this syntactic limitation. As for the leading/trailing comma for array and record syntax, I am sympathetic to the idea of supporting the trailing comma because one could copy-paste JS arrays/records into PureScript and more cases would compile despite the trailing comma. (This "problem" could also be resolved by making However, the current array/record syntax do not support the -- leading
[
, a
, b
]
-- vs
-- trailing
[
a,
b,
]If we did support a change, I would prefer supporting a leading delimiter for array/record syntax and not a trailing delimiter because it's a "smaller" change. However, I think such a change should only be made if it works in all contexts, including single or multi Phew! All that being said, I think this is where I stand:
|
|
First, from a purely what-does-this-enable-for-users perspective, let me note that there is absolutely no difference between leading and trailing delimiters. In particular, all of your numbered arguments for So the way I see this proposal is this:
I think 1 is a clear ‘yes’. I think the answer to 2 is most positive for sum type definitions and array/object literals and patterns, followed closely by object update expressions and row types; I also think a decent case can be made for import/export lists. I'm neutral on constructor lists within import lists, fundeps, compound constraints, and multi-head case expressions—I don't expect those to excite anyone, until the one user three years from now who'll say, ‘Hey, I'm designing a class with seventeen possible functional dependencies that I want to toggle and I don't know why I can't use (leading/trailing) commas there just like I use them with big arrays.’ 3 contains the details harboring the devil. Here's what I think I know:
As for the rest... I could see a mostly conservative proposal of only leading To some of your specific points:
Exported, but not imported. It's still legal, and sometimes useful, to import some but not all of a type's constructors; and so it's still possible to want to individually add and remove from that list, which means that ULS can still add (an admittedly small amount of) value here.
Just to clarify, the multi-head case issue has nothing to do with array/record patterns. These commas: case
, expr1
, expr2
of
, BigPattern1
, BigPattern2
-> ...
, BigPattern3 -- this is where the issue was
, BigPattern4
-> ...are not the same as these commas: case someArrayValuedExpression of
[
, BigPattern1
, BigPattern2
] -> ... |
|
I just saw some Haskell code where the imports had trailing commas. It felt a little weird and I agree this is mostly a bias on how ML-code should look like rather than something objective. This could be enforced with a formatter ( (but +1 leading pipe on ADTs specifically 😉) |
That is definitely the case.
I tried |
|
I would say that one thing to consider is to determine whether or not these changes would end up becoming the "recommended" formatting to use for writing PureScript. A few years down the line, would we see new documentation making use of and potentially recommending these optional formatting styles? I've no strong arguments about allowing trailing |
|
I am moderately 👎 on this, mainly because maximal syntactic expressiveness is not something I'm really interested in. Regarding what @toastal wrote:
I'm much more inclined to land an overall harmony. I'm least 👎 and perhaps somewhat 👍 on leading pipes, but I just don't really agree regarding commas. Trailing commas just don't scale well overall with our syntax. example = [
case foo of
...
Foo ->
... some big expression, -- <- so easy to miss
]The way to make that stand out, and not be so hard to miss would be to put a newline before the trailing comma, which I think is an even more bizarre style. I personally really don't want to see trailing commas be a thing. It would be a hard pass for me to support it in |
|
I think less syntax is better. Less variation to parse mentally, less things to think about when deciding how to format your own code, less things for tooling to have to deal with, so I'm not a fan of this change. That being said, I do think good multi-line syntax is important, and I think maybe the one case above where something that doesn't currently have a good multi-line syntax now, would, is case statements with leading commas. That being said. I'm not sure how often I use multi-line case statements in my code (potentially never), so I'm not sure I feel it's worth the extra syntax. |
|
I don't think this is a good idea, this adds even more inconsistency of code styles and makes things look drifting to JS style excessive flexibility. |
|
It seems like leading pipes are at least much less controversial than the rest of this PR. Maybe those can be extracted out into their own PR and discussed? Personally, I would really like to have leading pipes, but I don't have strong feelings about the rest. |
|
@natefaubion you bring up a good point with leading commas. It seems there's a stylistic reason to try to keep a consistent left-hand character which would match leading pipe. type Result e a =
| Success s → …
| Failure e → …I guess by that argument I could see an case for stylistic leading characters too like: burgerToppings ∷ Array String
burgerToppings =
[
, "bacon"
, "peanut butter"
, "fried banana"
]or burgerToppings ∷ Array String
burgerToppings = [
, "bacon"
, "peanut butter"
, "fried banana"
]This makes it easier to move/modify the first element, though it looks a little weird. |
|
An interesting tidbit, I find, is something I've liked in zig which supports trailing commas. |
|
I may have changed my stance about the comma situation. I'm definitely still pro-leading pipe above anything, but someone pointed out to me that leading commas doesn't play the best with tabs (as opposed to spaces). There's an open issue about tab support for accessibility, and I'm inclined to support it (because my eyes have had trouble reading 2-space (and 1-space) indentation for the last 2 years). The last thing you want to do is start mixing the two though. For [ a
, { b:
{ c: unit
, d: unit
}
}
}you could put tabs after the commas/opening symbols like [ a
, { b:
{ c: unit
, d: unit
}
}
}or maybe you embrace the trailing commas syntax (easiest to read IMO) [
a,
{
b: {
c: unit,
d: unit,
},
},
]The examples I last posted are I believe harder to read in this case. [
, a
, {
, b: {
, c: unit
, d: unit
}
}
}(if this looks bad and too spaced out in your browser, it's because because browsers don't make it easy to configure your preferred tab width, but a userStyle can cover it; I did raise as issue with Mozilla) |
Description of the change
Inspired by this Discourse post, I made a small patch to the CST to support optional leading and trailing delimiters in almost every location in the grammar where I didn't think it would cause problems.
The one place I was less than maximally liberal was in multi-headed
caseexpressions. When I tried to make extra delimiters optional in patterns, something prevented it from working in my tests—probably related to the fancy ad-hoc monadic parsing that is used in that part of the grammar.Example of what I'm talking about:
Rather than try to fix it, I chose not to enable extra delimiters in that part of the grammar; and then, for consistency, also removed support for extra delimiters from the head of the
caseas well (even though those worked fine). While this shortcoming is regrettable, I think it'd be quite low down on the list of places in the grammar where users would want to use extra delimiters.Also note that using leading and trailing delimiters together, a la
[,1,], is not supported anywhere. It would have been trivial to do so but I have standards, sir, standards!Despite submitting this PR and the voiced support for at least some of the syntax changes included here, I have two misgivings. The first is that I don't know if any of these changes break any assumptions made during the layout phase of lexing, which does seem to be sensitive to where commas appear. I haven't noticed any issues but I don't have a thorough understanding of that algorithm. The second is that, by cravenly refraining from picking a side between the delimiters-first versus delimiters-last styles of code formatting, I fear I may stoke the flames of utterly unproductive format wars. There is definitely an argument for having one clear way to do simple things in a language and I don't know how strongly that argument should apply here.
Regardless of the above reservations, this PR is ready for review and discussion, though I don't consider it all that urgent.
Checklist: