hexagoxel
/
brittany
1
0
Fork 0
Code Issues 117 Pull Requests 5 Packages Projects Releases 5 Wiki Activity

Support IndentPolicyLeft #66

Merged
eborden merged 23 commits from indentpolicyleft into dev 2017-12-01 00:30:28 +01:00
Conversation 28 Commits 23 Files Changed 9
eborden commented 2017-11-19 21:14:29 +01:00 (Migrated from github.com)

This PR introduces support for IndentPolicyLeft in brittany.

This work has proceeded with the intent to couple IndentPolicyLeft with ColumnAlignModeDisabled to allow a context free formatting style for brittany. Decisions on syntactic form follow that philosophy. Tests have been added for this use case. They currently represent a fair amount of duplication, but that should be able to be reduced once inline format annotations are available.

This PR introduces support for `IndentPolicyLeft` in `brittany`. This work has proceeded with the intent to couple `IndentPolicyLeft` with `ColumnAlignModeDisabled` to allow a context free formatting style for `brittany`. Decisions on syntactic form follow that philosophy. Tests have been added for this use case. They currently represent a fair amount of duplication, but that should be able to be reduced once inline format annotations are available.
🎉 2
eborden commented 2017-11-19 22:05:06 +01:00 (Migrated from github.com)

Actually there was less left in this batch of work then I realised.

This is ready for review.

For posterity this is addressing issues raised in: https://github.com/lspitzner/brittany/issues/56

Actually there was less left in this batch of work then I realised. This is ready for review. For posterity this is addressing issues raised in: https://github.com/lspitzner/brittany/issues/56
lspitzner commented 2017-11-20 21:56:13 +01:00 (Migrated from github.com)

In general this looks good. I still want to look at the different layouter changes in detail, and I won't do that today (even when the changes are relatively transparent).

But some first-glance stuff:

  • The test setup is reasonable, but I think in conflict with current dev branch. Shouldn't be too hard to fix though. Although the proper route is to add a commandline flag for indentpolicy and use the stuff from the inlineconfig branch.. but that can be improved at a later point too.
  • There is some code-duplication in Expr.hs. Well, admittedly there probably also was before this PR. I'll point out specifics in a closer review.
  • Some of the pending testcases that you changed are imo more about horizontal alignment than about indentation. Of course disabling horizontal alignment is in the spirit of #56, too. Maybe needs a bit clarification regarding the purpose of the tests (or maybe it is fine, too. just something i noticed on quick glance.)
In general this looks good. I still want to look at the different layouter changes in detail, and I won't do that today (even when the changes are relatively transparent). But some first-glance stuff: - The test setup is reasonable, but I think in conflict with current `dev` branch. Shouldn't be too hard to fix though. Although the proper route is to add a commandline flag for indentpolicy and use the stuff from the `inlineconfig` branch.. but that can be improved at a later point too. - There is some code-duplication in Expr.hs. Well, admittedly there probably also was before this PR. I'll point out specifics in a closer review. - Some of the pending testcases that you changed are imo more about horizontal alignment than about indentation. Of course disabling horizontal alignment is in the spirit of #56, too. Maybe needs a bit clarification regarding the purpose of the tests (or maybe it is fine, too. just something i noticed on quick glance.)
👍 1
tfausak commented 2017-11-24 17:41:45 +01:00 (Migrated from github.com)

I just want to say that I would drop hindent for brittany in a heartbeat if brittany never did any context-sensitive alignment. I feel like ghc-exactprint is a more solid foundation than haskell-src-exts, but at the same time I don't want code formatted as shown in #56. I can't speak to implementation details, but @eborden's test suite looks exactly like how I would format things by hand.

I just want to say that I would drop `hindent` for `brittany` in a heartbeat if `brittany` never did any context-sensitive alignment. I feel like `ghc-exactprint` is a more solid foundation than `haskell-src-exts`, but at the same time I don't want code formatted as shown in #56. I can't speak to implementation details, but @eborden's test suite looks exactly like how I would format things by hand.
ElvishJerricco commented 2017-11-24 17:46:31 +01:00 (Migrated from github.com)

Agreed. I have never found myself preferring context sensitive alignment.

Agreed. I have never found myself preferring context sensitive alignment.
lspitzner (Migrated from github.com) reviewed 2017-11-24 20:59:18 +01:00
lspitzner (Migrated from github.com) left a comment

I've looked through all changes; most are fine without further comments. I am not certain yet if all context-sensitive stuff is removed yet, but if there are remaining cases they can be fixed later.

Apart from the minor stuff I commented, the major annoyance is making this mergeable with the dev branch. There are some large conflicts already I noticed (but it looks worse than it is).

I think a rebase will be easier to manage than a merge.

I've looked through all changes; most are fine without further comments. I am not certain yet if all context-sensitive stuff is removed yet, but if there are remaining cases they can be fixed later. Apart from the minor stuff I commented, the major annoyance is making this mergeable with the `dev` branch. There are some large conflicts already I noticed (but it looks worse than it is). I think a rebase will be easier to manage than a merge.
src/Language/Haskell/Brittany/Internal/Layouters/Expr.hs
@ -481,0 +550,4 @@
]
)
, ( indentPolicy /= IndentPolicyLeft
, docLines
lspitzner (Migrated from github.com) commented 2017-11-24 20:05:00 +01:00

Please remove the duplication here. In the IndentPolicyLeft case, the docAlt can move further in, also removing some duplication.

Please remove the duplication here. In the `IndentPolicyLeft` case, the docAlt can move further in, also removing some duplication.
lspitzner (Migrated from github.com) commented 2017-11-24 20:20:32 +01:00

Doesn't the only-one-let-binder-case (above) also need adjustment?

Doesn't the only-one-let-binder-case (above) also need adjustment?
src/Language/Haskell/Brittany/Internal/Layouters/Stmt.hs
lspitzner (Migrated from github.com) commented 2017-11-24 20:22:56 +01:00

Could use docAltFilter here too, just to .. stay consistent :D

Could use `docAltFilter` here too, just to .. stay consistent :D
lspitzner (Migrated from github.com) commented 2017-11-24 20:28:10 +01:00

docSetBaseAndIndent there most likely produces hanging indentation (or at least) more-than-usual indentation. Probably need to disable this alt-branch, too.

`docSetBaseAndIndent` there most likely produces ~~hanging indentation~~ (or at least) more-than-usual indentation. Probably need to disable this alt-branch, too.
lspitzner commented 2017-11-24 21:27:02 +01:00 (Migrated from github.com)

A bit of nitpicking: "context sensitive" is ambiguous, but the way i read it here, the only way to be truly context-insensitive would be to remove all docAlt nodes (or, equivalently, to disable all but one alternative in every case). Because only then depth in syntax-tree corresponds to indentation exactly, and only then changes to siblings have no chance of affecting the layout of any node. And IndentPolicyLeft is far from that, and it is not what we want either, see the extreme example of truly context-free layouting in my brittany rationale.

What we want instead is a compromise between that and the "least newlines possible" layout. Of course the current default may not be the right compromise. Indeed, I am somewhat inclined to making the non-hanging-indentation layout the default. I just don't consider IndentPolicyLeft that different from a theoretical standpoint :p

A bit of nitpicking: "context sensitive" is ambiguous, but the way i read it here, the only way to be truly context-insensitive would be to remove all `docAlt` nodes (or, equivalently, to disable all but one alternative in every case). Because only then depth in syntax-tree corresponds to indentation exactly, and only then changes to siblings have no chance of affecting the layout of any node. And `IndentPolicyLeft` is far from that, and it is not what we want either, see the [extreme example of truly context-free layouting in my brittany rationale](http://hexagoxel.de/postsforpublish/posts/2017-10-28-brittany-rationale.html#function-and-operator-application). What we want instead is a compromise between that and the "least newlines possible" layout. Of course the current default may not be the right compromise. Indeed, I am somewhat inclined to making the non-hanging-indentation layout the default. I just don't consider IndentPolicyLeft that different from a theoretical standpoint :p
tfausak commented 2017-11-24 23:39:56 +01:00 (Migrated from github.com)

I don't know if this is the right place to have this discussion, but for me at least "context free" layout means that some expression doesn't depend on the previous expression for its indentation. For example, this is "context sensitive" to me:

listToMaybe :: [a]
            -> Maybe a

Whereas this is "context free":

listToMaybe :: [a]
  -> Maybe a

In particular it doesn't mean that every expression is always formatted the same way. Line length is still a concern. But when something forces a newline, the indentation is always "one tab" rather than aligning with the thing above it.

As I said before, the examples/tests that @eborden added here are exactly how I want things formatted. If there's a term other than "context free" I should be using, I'll be happy to use it instead.

I don't know if this is the right place to have this discussion, but for me at least "context free" layout means that some expression doesn't depend on the previous expression for its indentation. For example, this is "context sensitive" to me: ``` hs listToMaybe :: [a] -> Maybe a ``` Whereas this is "context free": ``` hs listToMaybe :: [a] -> Maybe a ``` In particular it doesn't mean that every expression is always formatted the same way. Line length is still a concern. But when something forces a newline, the indentation is always "one tab" rather than aligning with the thing above it. As I said before, the examples/tests that @eborden added here are exactly how I want things formatted. If there's a term other than "context free" I should be using, I'll be happy to use it instead.
lspitzner commented 2017-11-25 00:30:34 +01:00 (Migrated from github.com)

Ah, so only indentation, and only after newlines. I'd call this "allowing hanging indentation". Perhaps I have too much of the implementation in mind: "indentation after newlines" is just one of the choices of layouting. E.g. if the choices are 1) oneliner 2) multiline with hanging 3) multiline with default indentation, then even if you disable 2) the decision between 1 and 3 is "context sensitive" ("would this overflow 80 cols?" can only be answered in context, for pretty much the same definition of "context" you used.)

(All this does not stand in any way against merging this. I don't mind at all how the test file is named etc. The intend of the issue and the PR are clear and the addition is welcome.)

Ah, so only indentation, and only after newlines. I'd call this "allowing hanging indentation". Perhaps I have too much of the implementation in mind: "indentation after newlines" is just one of the choices of layouting. E.g. if the choices are 1) oneliner 2) multiline with hanging 3) multiline with default indentation, then even if you disable 2) the decision between 1 and 3 is "context sensitive" ("would this overflow 80 cols?" can only be answered in context, for pretty much the same definition of "context" you used.) (All this does not stand in any way against merging this. I don't mind at all how the test file is named etc. The intend of the issue and the PR are clear and the addition is welcome.)
eborden commented 2017-11-26 02:52:46 +01:00 (Migrated from github.com)

I've updated the code per your inline comments. Would you like me to rebase this over the dev branch and change the base of this PR?

I've updated the code per your inline comments. Would you like me to rebase this over the `dev` branch and change the base of this PR?
lspitzner commented 2017-11-26 19:15:32 +01:00 (Migrated from github.com)

@eborden Well I need these branches merged one way or another. But I can see about the rebase myself, I probably have a better idea of how to handle the conflicts.
(The CI failure is nonsense afaict.)

@eborden Well I need these branches merged one way or another. But I can see about the rebase myself, I probably have a better idea of how to handle the conflicts. (The CI failure is nonsense afaict.)
lspitzner commented 2017-11-26 21:23:40 +01:00 (Migrated from github.com)

Pushed the rebased branch to eborden-indentpolicyleft. I was hoping to be able to push directly to the PR.. but what i tried did not work.

There are still two bad testcases. Also I think the stack build does not compile. (The travis CI script is still broken and won't report error correctly.)

Pushed the rebased branch to `eborden-indentpolicyleft`. I was hoping to be able to push directly to the PR.. but what i tried did not work. There are still two bad testcases. Also I think the stack build does not compile. (The travis CI script is still broken and won't report error correctly.)
eborden commented 2017-11-27 15:55:20 +01:00 (Migrated from github.com)

@lspitzner I'll see about merging that branch into this today.

@lspitzner I'll see about merging that branch into this today.
lspitzner commented 2017-11-27 17:46:13 +01:00 (Migrated from github.com)

I should note that current dev branch (and the one i rebased onto) is slighly broken. nothing to do with the merge, but with the removal of either package.

I should note that current dev branch (and the one i rebased onto) is slighly broken. nothing to do with the merge, but with the removal of `either` package.
eborden commented 2017-11-27 17:54:58 +01:00 (Migrated from github.com)

@lspitzner Do you always want PRs submitted against the dev branch? If so that might be a good thing to outline in a contributors section in README.md.

@lspitzner Do you always want PRs submitted against the `dev` branch? If so that might be a good thing to outline in a contributors section in `README.md`.
lspitzner commented 2017-11-27 18:02:47 +01:00 (Migrated from github.com)

@eborden indeed, and i already added such a note to the readme a few days ago. bit late for this PR, but the rebase was still manageable :)

would you mind if i rebase again, just so we base this PR on a working commit?

@eborden indeed, and i already added such a note to the readme a few days ago. bit late for this PR, but the rebase was still manageable :) would you mind if i rebase again, just so we base this PR on a working commit?
eborden commented 2017-11-27 18:08:21 +01:00 (Migrated from github.com)

@lspitzner Do you want me to give you access to my fork so you can rebase this branch specifically?

@lspitzner Do you want me to give you access to my fork so you can rebase this branch specifically?
eborden commented 2017-11-27 18:09:57 +01:00 (Migrated from github.com)

@lspitzner Added you as a collaborator.

@lspitzner Added you as a collaborator.
lspitzner commented 2017-11-27 18:19:51 +01:00 (Migrated from github.com)

great, lets see what the CI says. With some luck it will properly report things this time :)

great, lets see what the CI says. With some luck it will properly report things this time :)
❤️ 1
lspitzner commented 2017-11-27 18:49:48 +01:00 (Migrated from github.com)

Ok, that looks right this time; the test failures there are exactly what i see locally. And I think both are just mistakes of the testcases themselves, right?

Ok, that looks right this time; the test failures there are exactly what i see locally. And I think both are just mistakes of the testcases themselves, right?
eborden commented 2017-11-27 18:58:58 +01:00 (Migrated from github.com)

Yup, that seems to be the case. I'll pull this and fix those up in a bit.

Yup, that seems to be the case. I'll pull this and fix those up in a bit.
👍 1
eborden (Migrated from github.com) reviewed 2017-11-27 23:18:15 +01:00
src/Language/Haskell/Brittany/Internal/Layouters/Stmt.hs
@ -59,0 +51,4 @@
LetStmt binds -> layoutLocalBinds binds >>= \case
Nothing -> docLit $ Text.pack "let" -- i just tested
-- it, and it is
-- indeed allowed.
eborden (Migrated from github.com) commented 2017-11-27 23:18:10 +01:00

Is there a simple way to enable this when the expression is actually a single line? That might be too much complexity thought 😄

Is there a simple way to enable this when the expression is actually a single line? That might be too much complexity thought :smile:
lspitzner (Migrated from github.com) reviewed 2017-11-27 23:29:57 +01:00
src/Language/Haskell/Brittany/Internal/Layouters/Stmt.hs
@ -59,0 +51,4 @@
LetStmt binds -> layoutLocalBinds binds >>= \case
Nothing -> docLit $ Text.pack "let" -- i just tested
-- it, and it is
-- indeed allowed.
lspitzner (Migrated from github.com) commented 2017-11-27 23:29:57 +01:00

Like this? I think this works.. oh I broke the tests.

Like this? I think this works.. oh I broke the tests.
lspitzner (Migrated from github.com) reviewed 2017-11-27 23:33:38 +01:00
src/Language/Haskell/Brittany/Internal/Layouters/Stmt.hs
@ -59,0 +51,4 @@
LetStmt binds -> layoutLocalBinds binds >>= \case
Nothing -> docLit $ Text.pack "let" -- i just tested
-- it, and it is
-- indeed allowed.
lspitzner (Migrated from github.com) commented 2017-11-27 23:33:38 +01:00

But they highlight the changes. And looks approximately correct (although the test coverage is not to be trusted entirely.)

But they highlight the changes. And looks approximately correct (although the test coverage is not to be trusted entirely.)
lspitzner (Migrated from github.com) reviewed 2017-11-27 23:42:09 +01:00
src/Language/Haskell/Brittany/Internal/Layouters/Stmt.hs
lspitzner (Migrated from github.com) commented 2017-11-27 23:42:09 +01:00

You could even say this is less complexity :)

You could even say this is less complexity :)
lspitzner commented 2017-11-30 23:22:37 +01:00 (Migrated from github.com)

@eborden you probably did not get notified when i commented on the diff comments. Any opinion on the last commit I added? Otherwise I'll assume this is ok and simply merge. (dev branch has diverged already again. not by much, but i really want this merged sooner rather than later.)

@eborden you probably did not get notified when i commented on the diff comments. Any opinion on the last commit I added? Otherwise I'll assume this is ok and simply merge. (dev branch has diverged already again. not by much, but i really want this merged sooner rather than later.)
eborden commented 2017-11-30 23:51:22 +01:00 (Migrated from github.com)

Oh woops. Sorry about that. That seems like a good approach. So will that fail if it can't force something into a single line?

Oh woops. Sorry about that. That seems like a good approach. So will that fail if it can't force something into a single line?
lspitzner commented 2017-11-30 23:56:03 +01:00 (Migrated from github.com)

Yeah. I'd phrase it as "it restricts the possible layouts for the argument-bridoc to those that fit in a single line" but that probably captures the same idea.

Yeah. I'd phrase it as "it restricts the possible layouts for the argument-bridoc to those that fit in a single line" but that probably captures the same idea.
eborden commented 2017-11-30 23:59:02 +01:00 (Migrated from github.com)

Awesome. That sounds perfect to me.

Awesome. That sounds perfect to me.
lspitzner commented 2017-12-01 00:05:00 +01:00 (Migrated from github.com)

(a sequence of docLit, docSeparator and (docForceSingleLine x) for arbitrary x thereby only has layouts that are single-line as a whole.)

Great, consider it merged then (I'll wait for CI).

Thanks for the contribution!

I'll make a 0.9.0.0 release that will include this in the next days.

(a sequence of docLit, docSeparator and (docForceSingleLine x) for arbitrary x thereby only has layouts that are single-line as a whole.) Great, consider it merged then (I'll wait for CI). Thanks for the contribution! I'll make a 0.9.0.0 release that will include this in the next days.
🎉 2
eborden commented 2017-12-01 15:58:51 +01:00 (Migrated from github.com)

Sweet, thanks for helping me through it!

Sweet, thanks for helping me through it!
tfausak commented 2017-12-03 23:25:08 +01:00 (Migrated from github.com)

FYI I re-formatted a medium-sized project with brittany and it went great: e88f4be14d?diff=split&w=1

Notes when compared to hindent:

  • brittany puts :: on a new line instead of on the same line as the identifier. x\n :: a versus x ::\n a. I prefer the way that brittany does it.
  • In general brittany uses a lot fewer newlines without making things cramped, particularly after ->, =, and <-.
  • brittany puts operators at the beginning of the line, which is pretty much always what I want.
  • brittany indents after let instead of aligning. Yay!

There were two things I noticed that felt weird. First, brittany always indents after let even if it would fit on one line. Also, it does the same thing after in. For example:

-- this is how i sometimes want things to look
let foo = bar
in foo baz
-- but brittany will space it out like this
let
  foo = bar
in
  foo baz

Not really a problem. It eats up some vertical space in the name of consistency. The other weird thing is long expression inside parentheses.

-- this is how i would normally format it
(foo
  bar
  baz)
-- this is what brittany does
( foo -- space after opening parenthesis
  bar
  baz
) -- closing parenthesis on new line

This feels weird but it's not really a problem. It's more consistent with multi-line tuples or lists, but I don't think I've ever seen Haskell code formatted like this before. On the other hand, it always bugged me that hindent formatted tuples like this:

( foo
, bar
, baz) -- closing parenthesis on new line

So maybe the problem is me 😄

FYI I re-formatted a medium-sized project with `brittany` and it went great: https://github.com/tfausak/rattletrap/commit/e88f4be14de8cad31ecf01f0b97309188b557394?diff=split&w=1 Notes when compared to `hindent`: - `brittany` puts `::` on a new line instead of on the same line as the identifier. `x\n :: a` versus `x ::\n a`. I prefer the way that `brittany` does it. - In general `brittany` uses a lot fewer newlines without making things cramped, particularly after `->`, `=`, and `<-`. - `brittany` puts operators at the beginning of the line, which is pretty much always what I want. - `brittany` indents after `let` instead of aligning. Yay! There were two things I noticed that felt weird. First, `brittany` always indents after `let` even if it would fit on one line. Also, it does the same thing after `in`. For example: ``` hs -- this is how i sometimes want things to look let foo = bar in foo baz -- but brittany will space it out like this let foo = bar in foo baz ``` Not really a problem. It eats up some vertical space in the name of consistency. The other weird thing is long expression inside parentheses. ``` hs -- this is how i would normally format it (foo bar baz) -- this is what brittany does ( foo -- space after opening parenthesis bar baz ) -- closing parenthesis on new line ``` This feels weird but it's not really a problem. It's more consistent with multi-line tuples or lists, but I don't think I've ever seen Haskell code formatted like this before. On the other hand, it always bugged me that `hindent` formatted tuples like this: ``` hs ( foo , bar , baz) -- closing parenthesis on new line ``` So maybe the problem is me 😄
eborden commented 2017-12-04 00:01:38 +01:00 (Migrated from github.com)

Hmm that let issue can likely be fixed. There is logic in the statement version that could be ported to the expression form. Could you open an issue?

Hmm that `let` issue can likely be fixed. There is logic in the statement version that could be ported to the expression form. Could you open an issue?
tfausak commented 2017-12-04 00:14:18 +01:00 (Migrated from github.com)

👉 https://github.com/lspitzner/brittany/issues/73

👉 https://github.com/lspitzner/brittany/issues/73
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
blocked: dependency
   
blocked: info-needed
   
bug
   
duplicate
   
enhancement
   
fixed in HEAD
   
help wanted
   
hs:arrows
   
hs:brackets
   
hs:classes
   
hs:comments
   
hs:do-notation
   
hs:guards
   
hs:lists
   
hs:operators
   
hs:patterns
   
hs:records
   
hs:types
   
invalid
   
language extension support
   
layouting
   
needs confirmation
   
priority: high
   
priority: low
   
question
   
revisit before next release
   
wontfix
No Label
blocked: dependency
blocked: info-needed
bug
duplicate
enhancement
fixed in HEAD
help wanted
hs:arrows
hs:brackets
hs:classes
hs:comments
hs:do-notation
hs:guards
hs:lists
hs:operators
hs:patterns
hs:records
hs:types
invalid
language extension support
layouting
needs confirmation
priority: high
priority: low
question
revisit before next release
wontfix
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
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#66
There is no content yet.
Delete Branch "indentpolicyleft"

Deleting a branch is permanent. Although the deleted branch may exist for a short time before cleaning up, in most cases it CANNOT be undone. Continue?