Suggestion: always indent to the right of the head #146

New Issue

2018-05-22T23:14:23+02:00

chreekat commented

2018-05-22 23:14:23 +02:00

(Migrated from github.com)

What I mean is that in situations where the head of an expression is allowed to be to the right of the body, don't do it, and create new lines if necessary.

No:

redirectRouteResponse status hdrs ra r req = emptyResponse
    status
    (locationHeader : hdrs)

Yes:

redirectRouteResponse status hdrs ra r req =
    emptyResponse status (locationHeader : hdrs)

I believe the top example is sometimes called "reverse indent", and I question its use in any situation.

Another way of phrasing this is, "Indent the entire expression first." If, after indenting the whole expression, more indenting is still needed, then so be it:

redirectRouteResponse status hdrs ra r req =
    emptyResponse
        (status xxy xxzz xxcc xciii xxx z cczz zz cccc zzz xx ccc)
        (locationHeader : hdrs)

What I mean is that in situations where the head of an expression is *allowed* to be to the right of the body, don't do it, and create new lines if necessary. No: ```haskell redirectRouteResponse status hdrs ra r req = emptyResponse status (locationHeader : hdrs) ``` Yes: ```haskell redirectRouteResponse status hdrs ra r req = emptyResponse status (locationHeader : hdrs) ``` I believe the top example is sometimes called "reverse indent", and I question its use in any situation. Another way of phrasing this is, "Indent the *entire* expression first." If, after indenting the whole expression, more indenting is still needed, then so be it: ```haskell redirectRouteResponse status hdrs ra r req = emptyResponse (status xxy xxzz xxcc xciii xxx z cczz zz cccc zzz xx ccc) (locationHeader : hdrs) ```

lspitzner commented

2018-05-23 19:42:55 +02:00

(Migrated from github.com)

Does this apply only to function applications? The brittany layouting construct connected to this would be "par-spacing", i.e. the docSetParSpacing and docForceParSpacing bits of the layouting DSL, where HsApp is not the only one including docSetParSpacing. Other users of this are e.g.:

func = do
  abc
func x = case x of
  ..
func = \case
  ..

(and there are probably some more if you search for either function name.)

Purely on the technical side, one can remove such invocations, but it is not possible to restrict specific interactions of two specific docSetParSpacing/docForceParSpacing uses.

Does this apply only to function applications? The brittany layouting construct connected to this would be "par-spacing", i.e. the `docSetParSpacing` and `docForceParSpacing` bits of the layouting DSL, where `HsApp` is not the only one including `docSetParSpacing`. Other users of this are e.g.: ~~~~.hs func = do abc func x = case x of .. func = \case .. ~~~~ (and there are probably some more if you search for either function name.) Purely on the technical side, one can remove such invocations, but it is not possible to restrict specific interactions of two specific `docSetParSpacing`/`docForceParSpacing` uses.

lspitzner commented

2018-05-23 19:45:24 +02:00

(Migrated from github.com)

phrased differently: the node to the right of "=" is currently allowed to have "par-spacing" and several layouters currently set the required "bit", and HsApp is only one of them.

phrased differently: the node to the right of "=" is currently allowed to have "par-spacing" and several layouters currently set the required "bit", and `HsApp` is only one of them.

chreekat commented

2018-05-24 19:12:23 +02:00

(Migrated from github.com)

Ah, interesting! You're right, I was just thinking of function application. (do is a funny case; I could almost get behind Chris Done's preference of not using par-spacing for it, too.)

Let me see if turning off par-spacing for HsApp does what I want.

Ah, interesting! You're right, I was just thinking of function application. (`do` is a funny case; I could *almost* get behind Chris Done's preference of not using par-spacing for it, too.) Let me see if turning off par-spacing for HsApp does what I want.

chreekat commented

2018-05-24 21:50:32 +02:00

(Migrated from github.com)

The complete list of cases:

HsLam (three sub-cases)
HsLamCase
HsApp (a couple alternatives in two sub-cases)
HsCase
HsIf
HsMultiIf
HsDo (DoExpr and MDoExpr)
RecordCon with fields; a couple sub-cases
RecordUpd

Of these, the ones I would want to change are HsApp and one case of RecordUpd. Here are my commentary on all the uses: cec0f19bcf

EDIT: New amended commit that removes superfluous comments.

The complete list of cases: * HsLam (three sub-cases) * HsLamCase * HsApp (a couple alternatives in two sub-cases) * HsCase * HsIf * HsMultiIf * HsDo (DoExpr and MDoExpr) * RecordCon with fields; a couple sub-cases * RecordUpd Of these, the ones I would want to change are HsApp and one case of RecordUpd. Here are my commentary on all the uses: https://github.com/chreekat/brittany/commit/cec0f19bcf9b9c30c070b400247aa91a807be650 EDIT: New amended commit that removes superfluous comments.

chreekat commented

2018-05-24 21:59:36 +02:00

(Migrated from github.com)

Removing some uses doesn't break any tests. I can't figure out what syntax is needed to exercise them. The use on line 183 is the only one I'd like to actually compare. Can you write a test that shows different behavior depending on whether or not line 183 gets commented out?

chreekat commented

2018-05-24 22:00:36 +02:00

(Migrated from github.com)

Oh, wait, that's one that looks easy to test. It has a comment already! I think the others are obvious enough.

chreekat commented

2018-05-24 22:20:40 +02:00

(Migrated from github.com)

No, I lied, I can't figure out how to test it.

chreekat commented

2018-06-02 22:04:00 +02:00

(Migrated from github.com)

I got busy, and I don't know if I'll make any headway on this any time soon.

lspitzner commented

2018-06-21 00:06:26 +02:00

(Migrated from github.com)

Sorry, I never followed up on this either. It still is on my to-do list to find those testcases you asked for, when I find time.

lspitzner commented

2018-07-06 23:08:57 +02:00

(Migrated from github.com)

@chreekat

Finally found some time (sorry). And after trying to find the difference line 183 makes, I have to admit that it probably is indeed superfluous, and this comes down to one detail that I had not mentioned yet:

The par-spacing flag is set implicitly for BriDoc values that are a sequence, where

All elements but the last element of the sequence are single-line
The last element is single-line or has the par-spacing flag set.

I.e. the "par-spacing" flag automatically moves upwards from the last element of a sequence to the sequence as a whole.

And in the stuff below line 183, we mandate the exact preconditions for this flag-forwarding, so we only permit layouts that already receive a forwarded par-spacing flag. I have added a comment about this in the code for now.

If it was your intention to disable this flag instance, this certainly does not make things easier. Disabling the forwarding is probably not what we want (without testing, i think there are several other cases that would be affected..), so the "simplest" solution would perhaps be a new node that forces the flag off again..

@chreekat Finally found some time (sorry). And after trying to find the difference line 183 makes, I have to admit that it probably is indeed superfluous, and this comes down to one detail that I had not mentioned yet: The par-spacing flag is set implicitly for BriDoc values that are a sequence, where 1. All elements but the last element of the sequence are single-line 2. The last element is single-line or has the par-spacing flag set. I.e. the "par-spacing" flag automatically moves upwards from the last element of a sequence to the sequence as a whole. And in the stuff below line 183, we mandate the exact preconditions for this flag-forwarding, so we only permit layouts that already receive a forwarded par-spacing flag. I have added a comment about this in the code for now. If it was your intention to disable this flag instance, this certainly does not make things easier. Disabling the forwarding is probably not what we want (without testing, i think there are several other cases that would be affected..), so the "simplest" solution would perhaps be a new node that forces the flag off again..

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