Consistency of RecordWildCards formatting #251

New Issue

2019-08-20T00:27:10+02:00

simonchatts commented

2019-08-20 00:27:10 +02:00

(Migrated from github.com)

Thanks for making Brittany!

Currently Brittany formats RecordWildCard pattern matches as {..} (no spaces) but expressions as { .. } (with spaces):

{-# LANGUAGE RecordWildCards #-}

data Record = Record { foo :: Int }

f1 :: IO Record
f1 = do
  Record {..} <- getRecord
  pure Record { .. } -- <=== note extra spaces


f2 :: Record -> ()
f2 (Record {..}) = ()

I have a (two-line) PR to make the expression also {..} but first wanted to check: would this be an acceptable PR? There's an explicit test for the current policy, but consistently no-space makes more sense to me.

(One step further: Personally, I can see the argument for omitting the leading space at all, to more clearly express how, eg, pure Record {..} is parsed as pure $ Record {..}, so is more explicit as pure Record{..}. But I can see how this would be a more controversial change, so don't want to de-rail a potentially simple discussion.)

Thanks for making Brittany! Currently Brittany formats RecordWildCard pattern matches as `{..}` (no spaces) but expressions as `{ .. }` (with spaces): ```haskell {-# LANGUAGE RecordWildCards #-} data Record = Record { foo :: Int } f1 :: IO Record f1 = do Record {..} <- getRecord pure Record { .. } -- <=== note extra spaces f2 :: Record -> () f2 (Record {..}) = () ``` I have a (two-line) PR to make the expression also `{..}` but first wanted to check: would this be an acceptable PR? There's an explicit test for the current policy, but consistently no-space makes more sense to me. (One step further: Personally, I can see the argument for omitting the leading space at all, to more clearly express how, eg, `pure Record {..}` is parsed as `pure $ Record {..}`, so is more explicit as `pure Record{..}`. But I can see how this would be a more controversial change, so don't want to de-rail a potentially simple discussion.)

👍 3

tfausak commented

2019-08-20 15:00:47 +02:00

(Migrated from github.com)

Thanks for reporting this! It sounds similar to (but different from) #126 and #223.

simonchatts commented

2019-08-20 19:04:18 +02:00

(Migrated from github.com)

Thanks for the pointers! Having had a look at those, it's still not obvious to me what the consistent policy would be here:

Is a RWC invocation more like using a regular record, in which case UseSpaces { .. }?
Or is it like the other use of (..) in module import/exports, which uses NoSpaces(..)?

I'm happy to submit a PR either way - any views? (Personally I prefer the latter, for the parsing rationale in the original comment, but I suspect the former may be more consistent...?)

Thanks for the pointers! Having had a look at those, it's still not obvious to me what the consistent policy would be here: - Is a RWC invocation more like using a regular record, in which case `UseSpaces { .. }`? - Or is it like the other use of `(..)` in module import/exports, which uses `NoSpaces(..)`? I'm happy to submit a PR either way - any views? (Personally I prefer the latter, for the parsing rationale in the original comment, but I suspect the former may be more consistent...?)

eborden commented

2019-08-22 16:35:26 +02:00

(Migrated from github.com)

There are three areas where space can exist, before the first bracket, after the first bracket and before the second bracket.

The reasonable options are:

Record{..}
Record {..}
Record { .. }
Record{ .. }

I'm partial to Record{..}, but that ship has likely sailed. I would argue in favor of Record { .. } since it is consistently spacious. As well it is consistent with placing spaces around brackets in single line record syntax.

Record { foo = _, bar = _ }

There are three areas where space can exist, before the first bracket, after the first bracket and before the second bracket. The reasonable options are: ```hs Record{..} Record {..} Record { .. } Record{ .. } ``` I'm partial to `Record{..}`, but that ship has likely sailed. I would argue in favor of `Record { .. }` since it is consistently spacious. As well it is consistent with placing spaces around brackets in single line record syntax. ```hs Record { foo = _, bar = _ } ```

lspitzner commented

2019-09-02 18:04:49 +02:00

(Migrated from github.com)

We should also consider Record{}. Together, we currently have

match Record{}            = 1
match Record {..}         = 2
match Record { a }        = 3
match Record { a, b }     = 4
match Record { a, ..}     = 5        -- inconsistent before closing }
match Record { a = True } = 6

construct 1 = Record{}
construct 2 = Record { .. }          -- inconsistent with pattern-match
construct 3 = Record { a }
construct 4 = Record { a, b }
construct 5 = Record { a , .. }      -- inconsistent before comma
construct 6 = Record { a = True }

update 1 = record{}
update 3 = record { a }
update 4 = record { a, b }
update 6 = record { a = True }

~~I think that consistency between pattern-matching and the corresponding expression is most important.~~ I think that in general, consistency between pattern-matching, construction and update is most important, i.e. the three items tagged above. Consistency between different patterns / different expressions would be nice (e.g. whether we have the space before "{"), and consistency with other constructs (lists, import statement parens etc.) comes after that.

(I just opened #257 which also discusses spaces (for lists instead of records). Slightly related.)

I'm partial to Record{..}, but that ship has likely sailed.

I still do currently consider this an option. Although I admit that the only consistent application of this would introduce a rather big change.

Can we get proposals for solutions to these inconsistencies, applied to the full example above?

We should also consider `Record{}`. Together, we currently have ~~~~.hs match Record{} = 1 match Record {..} = 2 match Record { a } = 3 match Record { a, b } = 4 match Record { a, ..} = 5 -- inconsistent before closing } match Record { a = True } = 6 construct 1 = Record{} construct 2 = Record { .. } -- inconsistent with pattern-match construct 3 = Record { a } construct 4 = Record { a, b } construct 5 = Record { a , .. } -- inconsistent before comma construct 6 = Record { a = True } update 1 = record{} update 3 = record { a } update 4 = record { a, b } update 6 = record { a = True } ~~~~ ~~I think that consistency between pattern-matching and the corresponding expression is most important.~~ I think that in general, consistency between pattern-matching, construction and update is most important, i.e. the three items tagged above. Consistency between different patterns / different expressions would be nice (e.g. whether we have the space before "{"), and consistency with other constructs (lists, import statement parens etc.) comes after that. (I just opened #257 which also discusses spaces (for lists instead of records). Slightly related.) > I'm partial to Record{..}, but that ship has likely sailed. I still do currently consider this an option. Although I admit that the only consistent application of this would introduce a rather big change. Can we get proposals for solutions to these inconsistencies, applied to the full example above?

lspitzner commented

2019-09-02 18:10:27 +02:00

(Migrated from github.com)

The "rather big change" would be the following:

match Record{}           = 1
match Record{..}         = 2
match Record{ a }        = 3
match Record{ a, b }     = 4
match Record{ a, .. }    = 5
match Record{ a = True } = 6

construct 1 = Record{}
construct 2 = Record{..}
construct 3 = Record{ a }
construct 4 = Record{ a, b }
construct 5 = Record{ a, .. }
construct 6 = Record{ a = True }

update 1 = record{}
update 3 = record{ a }
update 4 = record{ a, b }
update 6 = record{ a = True }

(2 vs 5 is still inconsistent, arguably.)

The "rather big change" would be the following: ~~~~.hs match Record{} = 1 match Record{..} = 2 match Record{ a } = 3 match Record{ a, b } = 4 match Record{ a, .. } = 5 match Record{ a = True } = 6 construct 1 = Record{} construct 2 = Record{..} construct 3 = Record{ a } construct 4 = Record{ a, b } construct 5 = Record{ a, .. } construct 6 = Record{ a = True } update 1 = record{} update 3 = record{ a } update 4 = record{ a, b } update 6 = record{ a = True } ~~~~ (2 vs 5 is still inconsistent, arguably.)

eborden commented

2019-09-03 21:03:11 +02:00

(Migrated from github.com)

I'm not against the "big change" as it binds the record identifier tightly with the opening bracket.

For the spacious bracket you'd have:

match Record {}           = 1
match Record { .. }       = 2
match Record { a }        = 3
match Record { a, b }     = 4
match Record { a, .. }    = 5
match Record { a = True } = 6

construct 1 = Record {}
construct 2 = Record { .. }
construct 3 = Record { a }
construct 4 = Record { a, b }
construct 5 = Record { a, .. }
construct 6 = Record { a = True }

update 1 = record {}
update 3 = record { a }
update 4 = record { a, b }
update 6 = record { a = True }

I'm not against the "big change" as it binds the record identifier tightly with the opening bracket. For the spacious bracket you'd have: ```hs match Record {} = 1 match Record { .. } = 2 match Record { a } = 3 match Record { a, b } = 4 match Record { a, .. } = 5 match Record { a = True } = 6 construct 1 = Record {} construct 2 = Record { .. } construct 3 = Record { a } construct 4 = Record { a, b } construct 5 = Record { a, .. } construct 6 = Record { a = True } update 1 = record {} update 3 = record { a } update 4 = record { a, b } update 6 = record { a = True } ```

eborden commented

2019-09-03 21:49:30 +02:00

(Migrated from github.com)

@lspitzner one way to avoid inconsistency on your previous layout is to have spacious wildcards, but tight identifier/bracket binding.

match Record{}           = 1
match Record{ .. }       = 2
match Record{ a }        = 3
match Record{ a, b }     = 4
match Record{ a, .. }    = 5
match Record{ a = True } = 6

construct 1 = Record{}
construct 2 = Record{ .. }
construct 3 = Record{ a }
construct 4 = Record{ a, b }
construct 5 = Record{ a, .. }
construct 6 = Record{ a = True }

update 1 = record{}
update 3 = record{ a }
update 4 = record{ a, b }
update 6 = record{ a = True }

In all examples I assume we are retaining the vertical layout

construct 7 = Record
  { a = True
  , b = False
  }

update 7 = record
  { a = True
  , b = False
  }

@lspitzner one way to avoid inconsistency on your previous layout is to have spacious wildcards, but tight identifier/bracket binding. ```hs match Record{} = 1 match Record{ .. } = 2 match Record{ a } = 3 match Record{ a, b } = 4 match Record{ a, .. } = 5 match Record{ a = True } = 6 construct 1 = Record{} construct 2 = Record{ .. } construct 3 = Record{ a } construct 4 = Record{ a, b } construct 5 = Record{ a, .. } construct 6 = Record{ a = True } update 1 = record{} update 3 = record{ a } update 4 = record{ a, b } update 6 = record{ a = True } ``` In all examples I assume we are retaining the vertical layout ```hs construct 7 = Record { a = True , b = False } update 7 = record { a = True , b = False } ```

👍 1

simonchatts commented

2019-09-03 22:17:16 +02:00

(Migrated from github.com)

There's also the

update 8 = record{ a = True }{ b = False }

class of case. Again, on balance, I think I come down on the ("big change") policy just quoted, since, eg:

pure record{ a = True }{ b = False }

does indeed parse as:

pure $ record { a = True } { b = False }

I don't see much code manually formatted with this convention today, but on balance I think it's clearer than the alternatives. And certainly theoretical inconsistency between that and

pure record{..}

is tolerable IMO.

This observation is in favour of the "big change", on the grounds that the whitespace usefully communicates binding precedence. (I'm willing to have a crack at implementing whatever policy the discussion concludes.)

There's also the ```haskell update 8 = record{ a = True }{ b = False } ``` class of case. Again, on balance, I think I come down on the ("big change") policy just quoted, since, eg: ```haskell pure record{ a = True }{ b = False } ``` does indeed parse as: ```haskell pure $ record { a = True } { b = False } ``` I don't see much code manually formatted with this convention today, but on balance I *think* it's clearer than the alternatives. And certainly theoretical inconsistency between that and ```haskell pure record{..} ``` is tolerable IMO. This observation is in favour of the "big change", on the grounds that the whitespace usefully communicates binding precedence. (I'm willing to have a crack at implementing whatever policy the discussion concludes.)

eborden commented

2019-09-03 23:25:39 +02:00

(Migrated from github.com)

This is the big win for me. The fact that record update/construction binds tighter than application makes removing the spurious space (between the identifier and the opening bracket) a positive visual cue.

pure record{ a = True }{ b = False }

does indeed parse as:

pure $ record { a = True } { b = False }

This is the big win for me. The fact that record update/construction binds tighter than application makes removing the spurious space (between the identifier and the opening bracket) a positive visual cue. > ```hs > pure record{ a = True }{ b = False } > ``` > does indeed parse as: > ```hs > pure $ record { a = True } { b = False } > ```

eborden commented

2019-09-07 17:54:59 +02:00

(Migrated from github.com)

@lspitzner given your 👍, it looks like you endorse this layout: https://github.com/lspitzner/brittany/issues/251#issuecomment-527611371

@simonchatts has proposed opening a PR, should he move forward with this layout?

@lspitzner given your :+1:, it looks like you endorse this layout: https://github.com/lspitzner/brittany/issues/251#issuecomment-527611371 ![image](https://user-images.githubusercontent.com/545655/64477177-e1162780-d15d-11e9-814f-ae64c5019e7d.png) @simonchatts has proposed opening a PR, should he move forward with this layout?

lspitzner commented

2019-09-07 19:30:55 +02:00

(Migrated from github.com)

Yes, I am fine with merging a PR with the layout from #251 (comment). It seems certainly a bit more consistent than what we have at the moment.

As we are still pre 1.0, I say we accept this breaking change, even without making it configurable. Should people dislike this change, we can always come back and add a second option behind a config flag that toggles Record{ vs Record {.

@simonchatts thanks for offering to work on this! Feel free to contact me if you have any questions on the implementation.

Yes, I am fine with merging a PR with the layout from [#251 (comment)](https://github.com/lspitzner/brittany/issues/251#issuecomment-527611371). It seems certainly a bit more consistent than what we have at the moment. As we are still pre 1.0, I say we accept this breaking change, even without making it configurable. Should people dislike this change, we can always come back and add a second option behind a config flag that toggles `Record{` vs `Record {`. @simonchatts thanks for offering to work on this! Feel free to contact me if you have any questions on the implementation.

eborden commented

2019-09-07 21:38:33 +02:00

(Migrated from github.com)

As we are still pre 1.0, I say we accept this breaking change, even without making it configurable. Should people dislike this change, we can always come back and add a second option behind a config flag that toggles Record{ vs Record {.

Agreed on both points.

> As we are still pre 1.0, I say we accept this breaking change, even without making it configurable. Should people dislike this change, we can always come back and add a second option behind a config flag that toggles Record{ vs Record {. Agreed on both points.

simonchatts commented

2020-08-07 09:12:55 +02:00

(Migrated from github.com)

@lspitzner I'm afraid Life has gotten in the way of this, and I won't be able to submit a good PR any time soon. It's probably best if you un-assign this, so that other interested folks aren't dissuaded from taking a shot.

Sign in to join this conversation.

No Milestone

No project

No Assignees

1 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: hexagoxel/brittany#251