brittany
brittany copied to clipboard
lspitzner
Use only whole multiples of indentAmount
Fix version: 0.11, using option "lconfig_indentPolicy: IndentPolicyMultiple"
By way of example, I would prefer the following:
-- conf_layout:
-- lconfig_indentAmount: 4
-- lconfig_indentPolicy: IndentPolicyLeft
-- lconfig_columnAlignMode: { tag: ColumnAlignModeDisabled }
test_all :: TestTree
test_all = testGroup
"all"
[ testCase "sanity" $ do
-- These lines indented 8 spaces (= 2 * 4)
filename <- Char8.pack <$> getDataFileName "test/data/small.webm"
prb <- avProbe filename =<< initAV
avProbeFormat prb @?= "matroska,webm"
avProbeStreams prb
@?= [(AVMediaTypeVideo, "vp8"), (AVMediaTypeAudio, "vorbis")]
]
For me, this is a question of ergonomics rather than style. With vim, I can easily indent lines to a multiple of indentAmount (aka shiftwidth), which means I can easily position a line to be valid Haskell before handing it off to brittany. But that only works if surrounding lines are already at a multiple of shiftwidth. Brittany currently formats the above to the following, which creates a problem.
test_all :: TestTree
test_all = testGroup
"all"
[ testCase "sanity" $ do
-- These lines indented 10 spaces
filename <- Char8.pack <$> getDataFileName "test/data/small.webm"
prb <- avProbe filename =<< initAV
avProbeFormat prb @?= "matroska,webm"
avProbeStreams prb
@?= [(AVMediaTypeVideo, "vp8"), (AVMediaTypeAudio, "vorbis")]
]
Adding a line with a new indent (like @?= ...), or particularly when returning to a previous indent (like if I appended a line to this do-block, which would need to be at 10 spaces), I'll have to add or remove spaces one-by-one, which is a small but nontrivial pain compared to blasting <c-t> or <c-d> and having everything just work.
(There is no problem adding lines at the same indent as its neighbors, thanks to vim's 'autoindent'.)
I grant that this feels like a corner case. I'm already off the default indentAmount (since shiftwidth=4 is the hill upon which I shall, in fact, perish). I'm cautiously hopeful that this may, however, be the spirit of the idea for IndentPolicyMultiple?
I'm cautiously hopeful that this may, however, be the spirit of the idea for IndentPolicyMultiple?
Correct. (And unfortunately for you, it is indeed not implemented to any degree.)
An optimistic first hunch is that this can be done with some clever but little changes in this code. At least that is the (first) place in the algorithm where you have access to the "current indentation" and could change the indentation amount accordingly. But I have not really thought this through yet. And I admit this part is not very well documented.
I'll think over this.
I did mean to ask about one thing. Given how IndentPolicyLeft has sort of morphed to be one component of "context independent layout", it seems that the semantics of IndentPolicyMultiple are somewhat orthogonal. Would it make sense to rethink IndentPolicy, perhaps decomposing into a couple independent options?
See 1c0f8b6811dde8716b2b3842f610c92a2831af89 on branch indent-policy-multiple.
This indeed produces (with inline-config we can now write this self-contained)
-- brittany { lconfig_indentAmount: 4, lconfig_indentPolicy: IndentPolicyLeft }
-- brittany { lconfig_columnAlignMode: { tag: ColumnAlignModeDisabled }}
test_all :: TestTree
test_all = testGroup
"all"
[ testCase "sanity" $ do
-- These lines indented 8 spaces (= 2 * 4)
filename <- Char8.pack <$> getDataFileName "test/data/small.webm"
prb <- avProbe filename =<< initAV
avProbeFormat prb @?= "matroska,webm"
avProbeStreams prb
@?= [(AVMediaTypeVideo, "vp8"), (AVMediaTypeAudio, "vorbis")]
]
But I am almost certain that this won't work in general because it does not even ensure that ((acp_indent + acp_indentPrep + indAdd) mod 4) = 0.
Just writing this i notice that this code breaks for
foo = do
let aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa =
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
foo
because it only reduces the additional indentation, where we need to add (i think).
Given how IndentPolicyLeft has sort of morphed to be one component of "context independent layout", it seems that the semantics of IndentPolicyMultiple are somewhat orthogonal. Would it make sense to rethink IndentPolicy, perhaps decomposing into a couple independent options?
But IndentPolicyMultiple seems to imply at least the "no hanging indentation" aspect of IndentPolicyLeft, perhaps excluding the layouting of imports/exports, right? Can you give some example(s) of "context sensitive layout" that makes sense for Multiple?
Just writing this i notice that this code breaks for
no wait, that was stupidity on my part. Pushed another commit to the branch. It seems to work even for
foo = do
let
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa =
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
+ aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
+ aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
foo
(only the newline after "let" is unfortunate, but that has nothing to do with this I believe)
Perhaps you can think of other more complex cases to test this on.
I think you're right that it would need to add indentation in the let aaaa.... = aaaa.... + aaaa.... case. Oh never mind, you just answered that yourself. :)
Can you give some example(s) of "context sensitive layout" that makes sense for Multiple?
The layouting of imports/exports is a good example. brittany's new default is practically unreadable to me, since my eye has to track across large amounts of whitespace. Whether or not someone wants Multiple, therefore would hopefully be independent of whether or not they want "compact" imports.[1]
It is also conceivable that someone who wants Multiple would be fine with this contrived example.
data XXXX = XXX { xx1 :: Int
, xx2 :: Int
}
However, that is definitely contrived, and not something I would want, myself.
[1]: I prefer an idiosyncratic 8-space indent for imports, which pleasingly puts the imports under the name of the module instead of under the keyword 'import', but that's neither here nor there. EDIT: I also use the 8 spaces for exports and data declarations, all of which are visible in the linked file
Under a let block is really the number one place I can think of. I'm going to try out your branch real quick.
This one's a bit funny. On the one hand, I do like where at 2 spaces, but on the other, that is the sort of flair that might be okay to sacrifice in the name of uniformity. Anyway, now we have both, which is probably not correct. Here are the brittany-created diffs:
pgExecute_ q = bracket open' PG.close (void . (`PG.execute_` q))
- where
- open' = PG.connectPostgreSQL ""
+ where open' = PG.connectPostgreSQL ""
versus the unchanged where in
+ , Model.PatronPaymentToken ==. Nothing
+ ]
assert (ct == 0)
where
- badSize (c, r, crwd) = counterexample (concat
I guess it makes sense to separate the imports-layouting mode from the indentation-policy. Bet this gives rise to some nice design questions of how flags imply other flags .. but perhaps a completely separated new flag is acceptable/preferable as well.
The only other issue I see is one that also exists for Left.
- let bucket = length targets `div` 5
+ let
+ bucket = length targets `div` 5
b0 = show (bucket * 5)
Perhaps fixing that is complicated, but it's a bit of a shame.
Ah, yeah i guess for where a different BriDoc constructor creates the indentation. I think this can fixed by just disabling the "where special" flag whatever the exact name was.
(Another case where we now want implication?..)
Notwithstanding those two little things, I am totally prepared to accept this existing behavior as the definition for IndentPolicyMultiple. :) Questions of small irregularities and hypothetical wishes for some sort of Free + Multiple can probably be deferred.
Great! the remaining work is of course to actually connect the Multiple flag instead of overwriting the Left behaviour. I fear this will touch some more code (grepping for IndentPolicyLeft gives a good amount of results). Also perhaps add some new testcases. (With the inline-config feature writing self-contained tests should be much easier than before.)
I expect that you will do this stuff; it seems relatively straight-forward. Of course if there is anything unclear I am open for questions.
Great! I can work on this today, in fact. With this, I should be able to use brittany everywhere with no hesitation. :) Thanks for your efforts.
On Sun, Apr 22, 2018, 10:48 Lennart Spitzner [email protected] wrote:
Great! the remaining work is of course to actually connect the Multiple flag instead of overwriting the Left behaviour. I fear this will touch some more code (grepping for IndentPolicyLeft gives a good amount of results). Also perhaps add some new testcases. (With the inline-config feature writing self-contained tests should be much easier than before.)
I expect that you will do this stuff; it seems relatively straight-forward. Of course if there is anything unclear I am open for questions.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/lspitzner/brittany/issues/135#issuecomment-383386772, or mute the thread https://github.com/notifications/unsubscribe-auth/AAg3qsiwn6NkID4j2SggUKxNugHlljGQks5trJgkgaJpZM4TXSvJ .
Well, I can't seem to correctly add failing tests. Can you look at https://github.com/lspitzner/brittany/commit/2ba97c013563aacaa95ec75132fc4fc1122b6556 and see what I'm doing wrong? I admit there's some guesswork happening.
Otherwise, you can see all changes at https://github.com/lspitzner/brittany/compare/master...chreekat:IndentPolicyMultiple?expand=1 .
I may have had some weird caching happening while experimenting with cabal new-test. That might explain things. Now I see that I've guessed wrong at extracting indentation policy. Pointers?
@chreekat my bad, tests were effectively disabled since one commit on master. This is fixed now.
@chreekat ah, good to hear! i was just looking into this as well, because brittany is blocking the ghc-8.4 support for HIE so i want to make a release really soon.
The changes look fine so far, what is it you want to change (apart from perhaps rebasing and squashing those first commits)? After rebasing, the tests seem to be clean after the obvious changes.
Great! It may, in fact, be basically done. I was having system-related issues with compiling and running tests. If you want to finish it up, go for it, otherwise I should have time this evening.
Fixed one bug i found when indentAmount>4, fixed the "let" issue (hopefully), cleaned up, merged into master, prepared release of 0.11. @chreekat if you want, check that all works as expected. Planing to do the release tomorrow.
Great! If I see any problems, I will let you know. Thanks for the assistance.
fixed the "let" issue (hopefully),
I don't know if this is worthy of a new issue, but brittany 0.11 produces the following:
-- brittany { lconfig_indentAmount: 4, lconfig_indentPolicy: IndentPolicyMultiple }
-- brittany { lconfig_columnAlignMode: { tag: ColumnAlignModeDisabled }}
withHandler waiReq identity h =
let (handler :: ReaderT RequestContext IO a) = unHandler h
in
withReaderT
(\(c :: ActionContext) -> RequestContext c waiReq identity)
handler
Alternately, it also produces this, when it's let that has multiple lines and in that has one:
-- brittany { lconfig_indentAmount: 4, lconfig_indentPolicy: IndentPolicyMultiple }
-- brittany { lconfig_columnAlignMode: { tag: ColumnAlignModeDisabled }}
withHandler waiReq identity h =
let
handler :: ReaderT RequestContext IO a = unHandler h
reqCtx :: ActionContext -> RequestContext
reqCtx c = RequestContext c waiReq identity
in withReaderT reqCtx handler
A missed case?
redirectRouteResponse s h r a req = emptyResponse
s
(( hLocation
, BSL.toStrict
(BSB.toLazyByteString (actionURL (Just req) r a (Wai.queryString req)))
)
: h
)
Another. Must be the same case as my previous comment:
-- brittany { lconfig_indentAmount: 4, lconfig_indentPolicy: IndentPolicyMultiple }
-- brittany { lconfig_columnAlignMode: { tag: ColumnAlignModeDisabled }}
xx success Slot { slotContainer = c, slotSegment = seg } =
((\p ->
(Data.String.fromStringggggggggggggggggggggggggggggggggggggggg
"xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"
)
)
)
It seems like this has been fixed in general, but there are still a few edge cases to clean up. Is that accurate?
@tfausak seems accurate to me. My personal policy would have dictated opening new issues, so if you're thinking the same...