Extra space after opening parenthesis #87

New Issue

2017-12-21T19:04:45+01:00

chreekat commented

2017-12-21 19:04:45 +01:00

(Migrated from github.com)

brittany produces the following:

x = fmap
  (map entityVal)
  ( selectList [Model.CrowdmatchHistoryPatron ==. pid]
               [Asc Model.CrowdmatchHistoryDate]
  )

I would expect this instead:

x = fmap
  (map entityVal)
  (selectList [Model.CrowdmatchHistoryPatron ==. pid]
              [Asc Model.CrowdmatchHistoryDate])

The location of the closing parenthesis, of course, could be a matter of taste. But that opening space seems to indicate brittany is expecting a tuple rather than a precedence-defining, single-element parenthetical.

brittany produces the following: ``` x = fmap (map entityVal) ( selectList [Model.CrowdmatchHistoryPatron ==. pid] [Asc Model.CrowdmatchHistoryDate] ) ``` I would expect this instead: ``` x = fmap (map entityVal) (selectList [Model.CrowdmatchHistoryPatron ==. pid] [Asc Model.CrowdmatchHistoryDate]) ``` The location of the closing parenthesis, of course, could be a matter of taste. But that opening space seems to indicate brittany is expecting a tuple rather than a precedence-defining, single-element parenthetical.

lspitzner commented

2017-12-21 22:24:43 +01:00

(Migrated from github.com)

hmm, the space could be changed using

@@ -327,7 +327,7 @@ layoutExpr lexpr@(L _ expr) = do
           ]
         , docSetBaseY $ docLines
           [ docCols ColOpPrefix
-            [ docParenLSep
+            [ docLit $ Text.pack "("
             , docAddBaseY (BrIndentSpecial 2) innerExpDoc
             ]
           , docLit $ Text.pack ")"

and that seems indeed nicer.
The closing parenthesis is a different matter, for cases like

layoutIE lie@(L _ ie) = a
  (  intersperse
      docCommaSep
      (map ((docLit =<<) . lrdrNameToTextAnn . prepareName) nsaaaaaaaaaa)
  ++ intersperse docCommaSep (map prepareFL fs)
  )

where the alternative would be..

layoutIE lie@(L _ ie) = a
  (intersperse
      docCommaSep
      (map ((docLit =<<) . lrdrNameToTextAnn . prepareName) nsaaaaaaaaaa)
    ++ intersperse docCommaSep (map prepareFL fs))

right? or perhaps

layoutIE lie@(L _ ie) = a
  (  intersperse
      docCommaSep
      (map ((docLit =<<) . lrdrNameToTextAnn . prepareName) nsaaaaaaaaaa)
  ++ intersperse docCommaSep (map prepareFL fs))

I think i like the first option the most. There is a more fundamental reason for this behaviour, too: It is really hard to decide (with brittanys algorithm) if there is sufficient space in the last line of the child node to append the closing parenthesis. Possible to adapt the algorithm, but a good bit of low-level changes.

I'll wait for feedback for a bit; default action will be to apply the above diff resulting in these layouts:

x = fmap
  (map entityVal)
  (selectList [Model.CrowdmatchHistoryPatron ==. pid]
              [Asc Model.CrowdmatchHistoryDate]
  )

layoutIE lie@(L _ ie) = a
  (  intersperse
      docCommaSep
      (map ((docLit =<<) . lrdrNameToTextAnn . prepareName) ns)
  ++ intersperse docCommaSep (map prepareFL fs)
  )

hmm, the space could be changed using ~~~~ @@ -327,7 +327,7 @@ layoutExpr lexpr@(L _ expr) = do ] , docSetBaseY $ docLines [ docCols ColOpPrefix - [ docParenLSep + [ docLit $ Text.pack "(" , docAddBaseY (BrIndentSpecial 2) innerExpDoc ] , docLit $ Text.pack ")" ~~~~ and that seems indeed nicer. The closing parenthesis is a different matter, for cases like ~~~~.hs layoutIE lie@(L _ ie) = a ( intersperse docCommaSep (map ((docLit =<<) . lrdrNameToTextAnn . prepareName) nsaaaaaaaaaa) ++ intersperse docCommaSep (map prepareFL fs) ) ~~~~ where the alternative would be.. ~~~~.hs layoutIE lie@(L _ ie) = a (intersperse docCommaSep (map ((docLit =<<) . lrdrNameToTextAnn . prepareName) nsaaaaaaaaaa) ++ intersperse docCommaSep (map prepareFL fs)) ~~~~ right? or perhaps ~~~~.hs layoutIE lie@(L _ ie) = a ( intersperse docCommaSep (map ((docLit =<<) . lrdrNameToTextAnn . prepareName) nsaaaaaaaaaa) ++ intersperse docCommaSep (map prepareFL fs)) ~~~~ I think i like the first option the most. There is a more fundamental reason for this behaviour, too: It is really hard to decide (with brittanys algorithm) if there is sufficient space in the last line of the child node to append the closing parenthesis. Possible to adapt the algorithm, but a good bit of low-level changes. I'll wait for feedback for a bit; default action will be to apply the above diff resulting in these layouts: ~~~~.hs x = fmap (map entityVal) (selectList [Model.CrowdmatchHistoryPatron ==. pid] [Asc Model.CrowdmatchHistoryDate] ) layoutIE lie@(L _ ie) = a ( intersperse docCommaSep (map ((docLit =<<) . lrdrNameToTextAnn . prepareName) ns) ++ intersperse docCommaSep (map prepareFL fs) ) ~~~~

chreekat commented

2017-12-21 22:32:59 +01:00

(Migrated from github.com)

Proposed action sounds great to me.

eborden commented

2017-12-23 17:16:20 +01:00

(Migrated from github.com)

I'm in favor of keeping the line break before the closing paren.

lspitzner commented

2017-12-28 20:48:11 +01:00

(Migrated from github.com)

Implemented as proposed.

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