Record update vs create has inconsistent bracing #126

New Issue

2018-03-14T16:47:02+01:00

pbrisbin commented

2018-03-14 16:47:02 +01:00

(Migrated from github.com)

I ran brittany (0.9.0.0) over bugsnag-haskell and observed the following two diffs:

Record create:

-bugsnagSession = BugsnagSession
-    { bsUser = Nothing
-    , bsContext = Nothing
-    , bsMetaData = Nothing
-    }
+bugsnagSession =
+    BugsnagSession {bsUser = Nothing, bsContext = Nothing, bsMetaData = Nothing}

Notice {bsUser.

Record update:

 updateException :: (BugsnagException -> BugsnagException) -> BeforeNotify
-updateException f event = event
-    { beExceptions = f <$> beExceptions event
-    }
+updateException f event = event { beExceptions = f <$> beExceptions event }

Notice { beExceptions.

I'd expect these two cases to be handled consistently w.r.t to whitespace within the braces. FWIW, I prefer the version with the whitespace.

Thanks for the awesome project btw, I'm totally down to get involved myself and I've seen you're great at supporting contributors. So if you agree this is a bug and want to point me in the right direction to fix it, please do :)

I ran brittany (0.9.0.0) over [bugsnag-haskell](https://github.com/pbrisbin/bugsnag-haskell) and observed the following two diffs: Record create: ```diff -bugsnagSession = BugsnagSession - { bsUser = Nothing - , bsContext = Nothing - , bsMetaData = Nothing - } +bugsnagSession = + BugsnagSession {bsUser = Nothing, bsContext = Nothing, bsMetaData = Nothing} ``` Notice `{bsUser`. Record update: ```diff updateException :: (BugsnagException -> BugsnagException) -> BeforeNotify -updateException f event = event - { beExceptions = f <$> beExceptions event - } +updateException f event = event { beExceptions = f <$> beExceptions event } ``` Notice `{ beExceptions`. I'd expect these two cases to be handled consistently w.r.t to whitespace within the braces. FWIW, I prefer the version with the whitespace. --- Thanks for the awesome project btw, I'm totally down to get involved myself and I've seen you're great at supporting contributors. So if you agree this is a bug and want to point me in the right direction to fix it, please do :)

lspitzner commented

2018-03-15 23:07:09 +01:00

(Migrated from github.com)

Yeah I agree that the current behaviour is inconsistent. RecordUpd/RecordCon should have the same spacing. This remind me of #87, which was about the spacing for (parentheses)/tuples, where the current conclusion is roughly:

space for multiple-line tuples for alignment of all elements, no space for any single-line tuples or (single or multiline) simple parentheses.

So one might argue for the parallel choices for records (no spaces for single-line updates). But my personal preference matches yours, i.e. always whitespace for records.

Thanks for the offer. I think that in this case at least the implementation is trivial (basically need to insert/remove appSep or docSeparator in two lines in Expr.hs). The hard part is the decision about which layout to choose. It always feels a bit strange for me to have the last word, as my preferences are not better than those of other haskell devs. Perhaps the best help would be to update/revise the "rationale" document I have over here. That description over there does not match the current implementation any more. But I think it would be useful to maintain that document for future reference.

And now I noticed that I a) never announced that document (did I?) b) do not link to it from the readme c) its source link does not work. Oh and d) I should mention the frontrowed guide. Ah, I'll have to fix those (but not this night).

Yeah I agree that the current behaviour is inconsistent. RecordUpd/RecordCon should have the same spacing. This remind me of #87, which was about the spacing for (parentheses)/tuples, where the current conclusion is roughly: > space for multiple-line tuples for alignment of all elements, no space for any single-line tuples or (single or multiline) simple parentheses. So one might argue for the parallel choices for records (no spaces for single-line updates). But my personal preference matches yours, i.e. always whitespace for records. Thanks for the offer. I think that in this case at least the implementation is trivial (basically need to insert/remove `appSep` or `docSeparator` in two lines in Expr.hs). The hard part is the decision about which layout to choose. It always feels a bit strange for me to have the last word, as my preferences are not better than those of other haskell devs. Perhaps the best help would be to update/revise the ["rationale" document I have over here](http://hexagoxel.de/postsforpublish/posts/2017-10-28-brittany-rationale.html#non-newline-whitespace). That description over there does not match the current implementation any more. But I think it would be useful to maintain that document for future reference. And now I noticed that I a) never announced that document (did I?) b) do not link to it from the readme c) its source link does not work. Oh and d) I should mention the [frontrowed guide](https://github.com/frontrowed/guides/blob/master/haskell-style.md). Ah, I'll have to fix those (but not this night).

chreekat commented

2018-05-25 16:09:29 +02:00

(Migrated from github.com)

The hard part is the decision about which layout to choose. It always feels a bit strange for me to have the last word, as my preferences are not better than those of other haskell devs.

I think this is good advice: https://chrisdone.com/posts/hindent-5. (It's gratifying to me personally that he went with Tibbe style, which I personally prefer.) There is a good argument against having too many options, and as he says there, "The tooling made it so automatic, that I didn’t have to understand the style or make any style decisions. . . . I completely did a U-turn. So I’m hoping that much of the community can do so too and put aside their stylistic preferences and embrace a standard."

Anyway, +1 to spaces between curly braces.

> The hard part is the decision about which layout to choose. It always feels a bit strange for me to have the last word, as my preferences are not better than those of other haskell devs. I think this is good advice: https://chrisdone.com/posts/hindent-5. (It's gratifying to me personally that he went with Tibbe style, which I personally prefer.) There is a good argument against having too many options, and as he says there, "The tooling made it so automatic, that I didn’t have to understand the style or make any style decisions. . . . I completely did a U-turn. So I’m hoping that much of the community can do so too and put aside their stylistic preferences and embrace a standard." Anyway, +1 to spaces between curly braces.

lspitzner commented

2018-05-25 18:25:57 +02:00

(Migrated from github.com)

Added spaces around curly braces for RecordCon. Think this is consistent now.

@chreekat we did not consider making things configurable here - the question of the right "default" (or the "agreed-upon layout" presuming we don't allow configuration) still needs to be answered.

There is the unfortunate fact that with hindent-5.2.3 (5.2.5 being latest) record spacing usage matched that of brittany before this fix:

hindent output:

func1 = event {beExceptions = f <$> beExceptions event}

func2 = Event {beExceptions = f <$> beExceptions event}

func1 =
  event
  { beExceptions = f <$> beExceptions event
  , beExceptions = f <$> beExceptions event
  }

func2 =
  Event
  { beExceptions = f <$> beExceptions event
  , beExceptions = f <$> beExceptions event
  }

But I am not too sure what we should derive from this.

Added spaces around curly braces for `RecordCon`. Think this is consistent now. @chreekat we did not consider making things configurable here - the question of the right "default" (or the "agreed-upon layout" presuming we don't allow configuration) still needs to be answered. There is the unfortunate fact that with hindent-5.2.3 (5.2.5 being latest) record spacing usage matched that of brittany _before_ this fix: hindent output: ~~~~.hs func1 = event {beExceptions = f <$> beExceptions event} func2 = Event {beExceptions = f <$> beExceptions event} func1 = event { beExceptions = f <$> beExceptions event , beExceptions = f <$> beExceptions event } func2 = Event { beExceptions = f <$> beExceptions event , beExceptions = f <$> beExceptions event } ~~~~ But I am not too sure what we should derive from this.

lspitzner commented

2018-05-25 18:27:31 +02:00

(Migrated from github.com)

Oh, this is not entirely fixed in another aspect: consider this brittany output:

func1 = event { beExceptions = f <$> beExceptions event }
func2 = Event { beExceptions = f <$> beExceptions event }
func1 = event { beExceptions = f <$> beExceptions event
              , beExceptions = f <$> beExceptions event
              }
func2 = Event
  { beExceptions = f <$> beExceptions event
  , beExceptions = f <$> beExceptions event
  }

So at least when allowing hanging indentation, things still differ. I'll fix this later.

Oh, this is not entirely fixed in another aspect: consider this brittany output: ~~~~.hs func1 = event { beExceptions = f <$> beExceptions event } func2 = Event { beExceptions = f <$> beExceptions event } func1 = event { beExceptions = f <$> beExceptions event , beExceptions = f <$> beExceptions event } func2 = Event { beExceptions = f <$> beExceptions event , beExceptions = f <$> beExceptions event } ~~~~ So at least when allowing hanging indentation, things still differ. I'll fix this later.

chreekat commented

2018-05-25 20:38:14 +02:00

(Migrated from github.com)

My apologies—some of my comments were esoteric, unspecific, and not entirely related to this particular issue. :)

lspitzner commented

2018-05-25 22:16:35 +02:00

(Migrated from github.com)

@chreekat no worries :)

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#126