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

Add import and module support #83

Closed
sniperrifle2004 wants to merge 18 commits from import into dev
pull from: import
merge into: hexagoxel:dev
Conversation 32 Commits 18 Files Changed 9
sniperrifle2004 commented 2017-12-17 14:57:38 +01:00 (Migrated from github.com)

Adds import and module Layouters. I refer to the tests for the layout choices I made (I tried to keep close to the style used in the brittany source).

Things to note:

  • I was not able to figure out how to deal with the importColumn config value. I suspected the column limit should be altered locally, but I couldn't find a way to do that from the layouter.

  • I had to use what feels like a trick to get the comments of a module to keep appearing above the module, since using docWrapNode on the module doc caused them to appear right after the where.

  • I added tests to cover reasonably expected use cases and certain edge cases, but I do not guarantee that there aren't things I overlooked.

Adds import and module Layouters. I refer to the tests for the layout choices I made (I tried to keep close to the style used in the brittany source). Things to note: - I was not able to figure out how to deal with the importColumn config value. I suspected the column limit should be altered locally, but I couldn't find a way to do that from the layouter. - I had to use what feels like a trick to get the comments of a module to keep appearing above the module, since using docWrapNode on the module doc caused them to appear right after the where. - I added tests to cover reasonably expected use cases and certain edge cases, but I do not guarantee that there aren't things I overlooked.
👍 3 🎉 4
lspitzner commented 2017-12-17 21:43:34 +01:00 (Migrated from github.com)

Nice work! This looks (sufficiently) well-tested and the code looks fine too, so I could have merged as-is, really.

  • I was not able to figure out how to deal with the importColumn config value.

    Consider the alignment in Brittany.Internal.Prelude. All imports are in a certain column, and that is what I had in mind for the config value to express. I'll bikeshed a bit more below.

  • I had to use what feels like a trick to get the comments of a module to keep appearing above the module, since using docWrapNode on the module doc caused them to appear right after the where.

    Yeah that is a bit wonky, but I don't see a much better way. I refactored it to use a docSeq
    with an empty node which simplifies the logic a bit, but ultimately it is somewhat confusing that the comments before the module are in this Ann field - if ghc-exactprint has some thought-out logic which field it actually places comments in, this logic has escaped me so far.

    (and yes, this also means that there indeed are probably side-cases with unusual comment placements that break, but i don't mind too much.)

  • I added tests to cover reasonably expected use cases and certain edge cases, but I do not guarantee that there aren't things I overlooked.

    i think the important bits are covered. with regards to comments, there are a lot of untested side-cases around in general, unfortunately. But this should not stop this from getting merged.

I have done mostly some superficial refactoring, hope you don't mind. Mostly replacing let in with where or do let to reduce general indentation.

Nice work! This looks (sufficiently) well-tested and the code looks fine too, so I could have merged as-is, really. - > I was not able to figure out how to deal with the importColumn config value. Consider the alignment in [`Brittany.Internal.Prelude`](https://github.com/lspitzner/brittany/blob/master/src/Language/Haskell/Brittany/Internal/Prelude.hs). All imports are in a certain column, and that is what I had in mind for the config value to express. I'll bikeshed a bit more below. - > I had to use what feels like a trick to get the comments of a module to keep appearing above the module, since using docWrapNode on the module doc caused them to appear right after the where. Yeah that is a bit wonky, but I don't see a much better way. I refactored it to use a `docSeq` with an empty node which simplifies the logic a bit, but ultimately it is somewhat confusing that the comments before the module are in this `Ann` field - if `ghc-exactprint` has some thought-out logic which field it actually places comments in, this logic has escaped me so far. (and yes, this also means that there indeed are probably side-cases with unusual comment placements that break, but i don't mind too much.) - > I added tests to cover reasonably expected use cases and certain edge cases, but I do not guarantee that there aren't things I overlooked. i think the important bits are covered. with regards to comments, there are a lot of untested side-cases around in general, unfortunately. But this should not stop this from getting merged. I have done mostly some superficial refactoring, hope you don't mind. Mostly replacing `let in` with `where` or `do let` to reduce general indentation.
lspitzner commented 2017-12-17 22:14:04 +01:00 (Migrated from github.com)

As for the bikeshedding: I think I prefer to have all imports layouted in a certain way, that is

--     111111111 22222222222222222222222222222222 3333333333333333333
import           Foo
import           Foo                            ( foo )
import qualified Foo
import           Foo                           as Foo
import           Foo                            ( foo
                                                , bar
                                                )

where the columns as marked are "reserved" for certain things: 1 for the potential "qualified" keyword, 2 for the module name, 3 for the binding names. further:

  • for more "special" qualifiers ({-# SOURCE #-} etc.) I would not mind reverting to some less constrained approach. But those are rather rare.
  • If module name is too long, newline, indent to col 50 again and place entities.
  • keep the space for qualified keyword even if there are no qualified imports yet in this module (for general consistency).
  • I essentially vote for hanging indentation here, but only because the usual downsides don't apply: Because we have absolute hanging indentation, local changes are not at risk of affecting surrounding code (and causing large diffs).

If that is the goal, the current approach of using docCols is not perfect in a couple of places. For example, the Import module header would currently be formatted like this:

module Language.Haskell.Brittany.Internal.Layouters.Import (layoutImport) where

#include "prelude.inc"

import Language.Haskell.Brittany.Internal.Types
-- ..
import RdrName (RdrName(..))
import GHC
  ( unLoc
  , runGhc
  , GenLocated(L)
  , moduleNameString
  , AnnKeywordId(..)
  , Located
  )
import           HsSyn
-- ..
import qualified FastString
import           BasicTypes

RdrName(..) and unLoc are not aligned, and qualified only affects stuff up to GHC (a side-effect of how docCols is currently configured to work).

Stylish-haskell seems to have sensible behaviour for imports, too. I'd have to do some more testing with it to figure out how they treat some of the more special cases.

(a good alternative for docCols might be to use docEnsureIndent.)

As for the bikeshedding: I think I prefer to have all imports layouted in a certain way, that is ~~~~.hs -- 111111111 22222222222222222222222222222222 3333333333333333333 import Foo import Foo ( foo ) import qualified Foo import Foo as Foo import Foo ( foo , bar ) ~~~~ where the columns as marked are "reserved" for certain things: 1 for the potential "qualified" keyword, 2 for the module name, 3 for the binding names. further: - for more "special" qualifiers (`{-# SOURCE #-}` etc.) I would not mind reverting to some less constrained approach. But those are rather rare. - If module name is too long, newline, indent to col 50 again and place entities. - keep the space for `qualified` keyword even if there are no qualified imports yet in this module (for general consistency). - I essentially vote for hanging indentation here, but only because the usual downsides don't apply: Because we have _absolute_ hanging indentation, local changes are _not_ at risk of affecting surrounding code (and causing large diffs). If that is the goal, the current approach of using `docCols` is not perfect in a couple of places. For example, the `Import` module header would currently be formatted like this: ~~~~.hs module Language.Haskell.Brittany.Internal.Layouters.Import (layoutImport) where #include "prelude.inc" import Language.Haskell.Brittany.Internal.Types -- .. import RdrName (RdrName(..)) import GHC ( unLoc , runGhc , GenLocated(L) , moduleNameString , AnnKeywordId(..) , Located ) import HsSyn -- .. import qualified FastString import BasicTypes ~~~~ `RdrName(..)` and `unLoc` are not aligned, and `qualified` only affects stuff up to `GHC` (a side-effect of how `docCols` is currently configured to work). Stylish-haskell seems to have sensible behaviour for imports, too. I'd have to do some more testing with it to figure out how they treat some of the more special cases. (a good alternative for `docCols` might be to use `docEnsureIndent`.)
sniperrifle2004 commented 2017-12-17 23:12:19 +01:00 (Migrated from github.com)

Ah, I see. I can do that. I am a little confused by the Foo winding up in the third column. I would have expected it to belong in column two. What about an aliased import with bindings? And what about hiding?

Ah, I see. I can do that. I am a little confused by the `Foo` winding up in the third column. I would have expected it to belong in column two. What about an aliased import with bindings? And what about hiding?
lspitzner commented 2017-12-17 23:44:15 +01:00 (Migrated from github.com)

It is for cases like

import qualified Brick.ReflexMain as Brick
import qualified Reflex as R
import qualified Reflex.Host.Class as R
import qualified Reflex.PerformEvent.Base as R
import qualified Reflex.Host.App as RH
import qualified Graphics.Vty as V
import qualified Control.Monad.Ref as Ref

where you cannot the actual names at all on a glance. Seems to be much nicer as

import qualified Brick.ReflexMain              as Brick
import qualified Reflex                        as R
import qualified Reflex.Host.Class             as R
import qualified Reflex.PerformEvent.Base      as R
import qualified Reflex.Host.App               as RH
import qualified Graphics.Vty                  as V
import qualified Control.Monad.Ref             as Ref

For hiding and qualified+list: not sure. perhaps

import           Foo                           as F
import           Foo                           as F        ( abc )
import           Foo                           as F        ( abc
                                                           , def
                                                           )
import           Foo                           as F hiding ( abc )
import           Foo                           as F hiding ( abc
                                                           , def
                                                           )

import           Foo                            ( abc )
import           Foo                            ( abc
                                                , def
                                                )
import           Foo                     hiding ( abc )
import           Foo                     hiding ( abc
                                                , def
                                                )

but I think i mostly use import+whitelist, import+blacklist, import-qualified, and import-qualified-as. So I don't really know about the other combinations.

It is for cases like ~~~~.hs import qualified Brick.ReflexMain as Brick import qualified Reflex as R import qualified Reflex.Host.Class as R import qualified Reflex.PerformEvent.Base as R import qualified Reflex.Host.App as RH import qualified Graphics.Vty as V import qualified Control.Monad.Ref as Ref ~~~~ where you cannot the actual names at all on a glance. Seems to be much nicer as ~~~~.hs import qualified Brick.ReflexMain as Brick import qualified Reflex as R import qualified Reflex.Host.Class as R import qualified Reflex.PerformEvent.Base as R import qualified Reflex.Host.App as RH import qualified Graphics.Vty as V import qualified Control.Monad.Ref as Ref ~~~~ For `hiding` and `qualified`+list: not sure. perhaps ~~~~.hs import Foo as F import Foo as F ( abc ) import Foo as F ( abc , def ) import Foo as F hiding ( abc ) import Foo as F hiding ( abc , def ) import Foo ( abc ) import Foo ( abc , def ) import Foo hiding ( abc ) import Foo hiding ( abc , def ) ~~~~ but I think i mostly use import+whitelist, import+blacklist, import-qualified, and import-qualified-as. So I don't really know about the other combinations.
👍 1
lspitzner commented 2017-12-17 23:45:16 +01:00 (Migrated from github.com)

stylish-haskell turns the last example (plus some more items to test the linebreaking behaviour) into:

import           Foo as F
import           Foo as F (abc)
import           Foo as F (abc, def, def, def, def, def, def, def, def, def,
                           def, def, def)
import           Foo as F hiding (abc)
import           Foo as F hiding (abc, def, def, def, def, def, def, def, def,
                           def)

import           Foo (abc)
import           Foo (abc, def, def, def, def, def, def, def, def, def, def,
                      def, def, def)
import           Foo hiding (abc)
import           Foo hiding (abc, def, def, def, def, def, def, def, def, def,
                      def, def, def, def, def, def, def, def, def, def, def)
stylish-haskell turns the last example (plus some more items to test the linebreaking behaviour) into: ~~~~.hs import Foo as F import Foo as F (abc) import Foo as F (abc, def, def, def, def, def, def, def, def, def, def, def, def) import Foo as F hiding (abc) import Foo as F hiding (abc, def, def, def, def, def, def, def, def, def) import Foo (abc) import Foo (abc, def, def, def, def, def, def, def, def, def, def, def, def, def) import Foo hiding (abc) import Foo hiding (abc, def, def, def, def, def, def, def, def, def, def, def, def, def, def, def, def, def, def, def, def) ~~~~
sniperrifle2004 commented 2017-12-18 00:39:15 +01:00 (Migrated from github.com)

Well, even if the use cases are rare, the formatter has to do something, preferably nice.

How about expanding a little vertical space to maintain the horizontal alignment:

import qualified Foo                     hiding ( foo )
import qualified Foo                           as F
import qualified Foo                           as F
                                                ( foo )
import qualified Foo                           as F
                                         hiding ( foo )
import qualified Foo                           as F
import           Foo                     hiding ( foo )

This maintains the binding column entirely. The downside is that it makes it a little harder to see what module the binding under an alias belongs to.

It even looks reasonable if the module name overlaps the column:

import qualified FooFooFooFooFooFooFooFooFooFooFoo           
                                               as F
                                         hiding ( foo )
Well, even if the use cases are rare, the formatter has to do something, preferably nice. How about expanding a little vertical space to maintain the horizontal alignment: ```haskell import qualified Foo hiding ( foo ) import qualified Foo as F import qualified Foo as F ( foo ) import qualified Foo as F hiding ( foo ) import qualified Foo as F import Foo hiding ( foo ) ``` This maintains the binding column entirely. The downside is that it makes it a little harder to see what module the binding under an alias belongs to. It even looks reasonable if the module name overlaps the column: ```haskell import qualified FooFooFooFooFooFooFooFooFooFooFoo as F hiding ( foo ) ```
lspitzner commented 2017-12-18 00:41:30 +01:00 (Migrated from github.com)

I like that.

I like that.
lspitzner commented 2017-12-18 19:08:18 +01:00 (Migrated from github.com)

Looks good!

I noticed the staticDefaultConfig contains layoutColumn 60, but i used 50 above. Do you have an opinion on the "best" default? (If the default changes, the tests need to be adapted again, or we need to override the test config.) (this question does not block merging, to be clear.)

I noticed and fixed this case in the above commit:

module TestModule where
import Prelude ( (<$>) )

Two to-dos:

  • Applying it to brittany's Prelude.hs still gives errors due to omitted comments, unfortunately.
  • (mostly a reminder for myself) I would like to review if rdrNameToText vs lrdrNameToTextAnn could cause problems in any other cases. Probably not, because module names cannot be operators.
Looks good! I noticed the `staticDefaultConfig` contains layoutColumn 60, but i used 50 above. Do you have an opinion on the "best" default? (If the default changes, the tests need to be adapted again, or we need to override the test config.) (this question does not block merging, to be clear.) I noticed and fixed this case in the above commit: ~~~~.hs module TestModule where import Prelude ( (<$>) ) ~~~~ Two to-dos: - Applying it to brittany's `Prelude.hs` still gives errors due to omitted comments, unfortunately. - (mostly a reminder for myself) I would like to review if `rdrNameToText` vs `lrdrNameToTextAnn` could cause problems in any other cases. Probably not, because module names cannot be operators.
tfausak commented 2017-12-18 20:32:00 +01:00 (Migrated from github.com)

I’m excited for Brittany to get this functionality! Thanks for opening this pull request, @sniperrifle2004.

I looked over the context free test cases and they seem a little weird to me. I would expect no extra spaces when formatting imports in a context free manner. In other words, no spaces for qualified, no spaces to align import lists, and no spaces to align aliases.

Am I reading those test cases right?

I’m excited for Brittany to get this functionality! Thanks for opening this pull request, @sniperrifle2004. I looked over the context free test cases and they seem a little weird to me. I would expect no extra spaces when formatting imports in a context free manner. In other words, no spaces for `qualified`, no spaces to align import lists, and no spaces to align aliases. Am I reading those test cases right?
sniperrifle2004 commented 2017-12-18 20:52:51 +01:00 (Migrated from github.com)

@lspitzner There is obviously a trade off between room for module names and room for aliases/ThingsWith/long(ish) import bindings. I suspect twenty characters is a little tight for bindings, so I think 50 is probably better.

Concerning the lrdrNameToTextAnn issue, I think that there may be a potential instance in the ThingWith case of layoutIE (For classes with operators).

@lspitzner There is obviously a trade off between room for module names and room for aliases/ThingsWith/long(ish) import bindings. I suspect twenty characters is a little tight for bindings, so I think 50 is probably better. Concerning the lrdrNameToTextAnn issue, I think that there may be a potential instance in the ThingWith case of layoutIE (For classes with operators).
sniperrifle2004 commented 2017-12-18 21:01:56 +01:00 (Migrated from github.com)

@tfausak You are reading them right. The import layout is completely context free. The alignment is defined purely around the elements in the import node and the importColumn config value, and is not affected by the surrounding elements or the alignment rules. So the context free layout is the same as the layout with context.

@tfausak You are reading them right. The import layout is completely context free. The alignment is defined purely around the elements in the import node and the importColumn config value, and is not affected by the surrounding elements or the alignment rules. So the context free layout is the same as the layout with context.
tfausak commented 2017-12-18 21:47:53 +01:00 (Migrated from github.com)

I feel like the imports don't match the rest of the the context free tests. I get that it's not exactly context sensitive. We had a discussion about what that means before: https://github.com/lspitzner/brittany/pull/66#issuecomment-346903193

I would prefer some way to make Brittany format imports like this:

import Data.List (sort)
import Data.Maybe (fromMaybe)
import qualified Data.Map as Map
import Data.HashMap.Strict as HashMap

That is, don't align anything.

I feel like the imports don't match the rest of the the context free tests. I get that it's not exactly context sensitive. We had a discussion about what that means before: https://github.com/lspitzner/brittany/pull/66#issuecomment-346903193 I would prefer some way to make `Brittany` format imports like this: ``` hs import Data.List (sort) import Data.Maybe (fromMaybe) import qualified Data.Map as Map import Data.HashMap.Strict as HashMap ``` That is, don't align anything.
👍 2
sniperrifle2004 commented 2017-12-18 22:19:12 +01:00 (Migrated from github.com)

Well, it is always possible to add a configuration option for that.

Well, it is always possible to add a configuration option for that.
tfausak commented 2017-12-18 22:27:01 +01:00 (Migrated from github.com)

I thought that's what the "context free" tests were testing, which is why I remarked on them. Specifically the settings are IndentPolicyLeft and ColumnAlignModeDisabled.

I thought that's what the "context free" tests were testing, which is why I remarked on them. Specifically the settings are `IndentPolicyLeft` and `ColumnAlignModeDisabled`.
sniperrifle2004 commented 2017-12-18 22:43:17 +01:00 (Migrated from github.com)

Column alignment mode deals with alignment elements over multiple syntactic constructs, like binding lines and IndentPolicyLeft means that britanny may not increase the indentation level to improve alignment. And the spacing isn't technically indentation (if I understand the mechanics correctly) nor added by the britanny transformer. It is applied by the layouter itself.

Column alignment mode deals with alignment elements over multiple syntactic constructs, like binding lines and IndentPolicyLeft means that britanny may not increase the indentation level to improve alignment. And the spacing isn't technically indentation (if I understand the mechanics correctly) nor added by the britanny transformer. It is applied by the layouter itself.
lspitzner commented 2017-12-19 01:18:37 +01:00 (Migrated from github.com)

@tfausak @sniperrifle2004

I had a long-wided reply typed, but that took too long, so I'll make it brief: I think you are both correct. The current behaviour is context-free but the tests never were meant to test that (apparently i indeed should have demanded a different name in the other PR..)

We can always make a flag in the opposite direction: One that relaxes IndentPolicyLeft/ColumnAlignModeDisabled for import statement layouting. (Because the actual intention behind IndPolLeft and CAMDisabled was context-free-ness, and this layouting does not violate that. Cue some more discussion about definitions, meanings and intended meanings..)

@sniperrifle2004 it is up to you if you want to implement this additional behaviour. If not, I might be lazy and simply disable reformatting of imports in the presence of IndPolLeft or CAMDisabled. But then it probably is relatively straight-forward to actually implement it.. considering that, @tfausak maybe you want to contribute this bit?

@tfausak @sniperrifle2004 I had a long-wided reply typed, but that took too long, so I'll make it brief: I think you are both correct. The current behaviour _is_ context-free but the tests never were meant to test that (apparently i indeed _should have_ demanded a different name in the other PR..) We can always make a flag in the opposite direction: One that relaxes `IndentPolicyLeft`/`ColumnAlignModeDisabled` for import statement layouting. (Because the actual intention behind IndPolLeft and CAMDisabled was context-free-ness, and this layouting does not violate _that_. Cue some more discussion about definitions, meanings and intended meanings..) @sniperrifle2004 it is up to you if you want to implement this additional behaviour. If not, I might be lazy and simply disable reformatting of imports in the presence of IndPolLeft or CAMDisabled. But then it probably is relatively straight-forward to actually implement it.. considering that, @tfausak maybe you want to contribute this bit?
sniperrifle2004 commented 2017-12-19 01:22:12 +01:00 (Migrated from github.com)

I fixed the ommitted comments for the prelude, but it positions them a little awkwardly because it retains the absolute position for most of them

I fixed the ommitted comments for the prelude, but it positions them a little awkwardly because it retains the absolute position for most of them
lspitzner commented 2017-12-19 01:23:13 +01:00 (Migrated from github.com)

@sniperrifle2004 there is one further (special) case: Some very long identifier:

import           Foo                            ( loooooooooooooooooooooooooooooooooooong )

This overflows the column limit. I am fine with that, but it is maybe worth a testcase to make it explicit.

@sniperrifle2004 there is one further (special) case: Some very long identifier: ~~~~.hs import Foo ( loooooooooooooooooooooooooooooooooooong ) ~~~~ This overflows the column limit. I am fine with that, but it is maybe worth a testcase to make it explicit.
sniperrifle2004 commented 2017-12-19 01:34:06 +01:00 (Migrated from github.com)

@lspitzner @tfausak

I'll construct a condensed layout based on the indent policy being IndentPolicyLeft. Would such a layout also try to keep bindings on the import line? Or would it still expand them by default?

@lspitzner @tfausak I'll construct a condensed layout based on the indent policy being IndentPolicyLeft. Would such a layout also try to keep bindings on the import line? Or would it still expand them by default?
👍 1
lspitzner commented 2017-12-19 01:47:57 +01:00 (Migrated from github.com)

there still are these broken:

#test import-with-comments-2

import           Test                                     ( abc
                                                          , def
                                                          -- comment
                                                          )

#test import-with-comments-3

import           Test                                     ( abc
                                                          -- comment
                                                          )

I can fix those too if this comment stuff is becoming too annoying. Probably involves splitFirstLast. But I don't want to push while you work. (And I'll be sleeping now anyways.)

Would such a layout also try to keep bindings on the import line? Or would it still expand them by default?

You mean whether to permit import Foo (foo, bar) when it does not overflow? I'd default to "yes", but there may be counterarguments.

there still are these broken: ~~~~ #test import-with-comments-2 import Test ( abc , def -- comment ) #test import-with-comments-3 import Test ( abc -- comment ) ~~~~ I can fix those too if this comment stuff is becoming too annoying. Probably involves `splitFirstLast`. But I don't want to push while you work. (And I'll be sleeping now anyways.) > Would such a layout also try to keep bindings on the import line? Or would it still expand them by default? You mean whether to permit `import Foo (foo, bar)` when it does not overflow? I'd default to "yes", but there may be counterarguments.
sniperrifle2004 commented 2017-12-19 08:31:26 +01:00 (Migrated from github.com)

It isn't really. I want to get a feeling for how to deal with them, since I want to contribute more in the future so I had better learn how to deal with them. I am having difficulty anticipating both brittany and ghc-exact-print and improving bad results with more specific instructions to brittany is also harder than I expect.

Let's take your tests as an example:

#test import-with-comments-2

import           Test                                     ( abc
                                                          , def
                                                          -- comment
                                                          )

#test import-with-comments-3

import           Test                                     ( abc
                                                          -- comment
                                                          )

Both these comments end up on the (Located [LIE]) and not on the LIE for abc and def. This means that ghc-exact-print thinks that the comment belongs to the entire binding list or rather to the ) which semantically means the same thing. It goes even further than that:

#test import-with-comments-4


import           Test                                     ( abc
                                                          , def -- comment
                                                          )

I would consider any comments on the right of some element to belong to the element on its left (There might be a case, where there is a more semantically correct element directly below the comment), but ghc-exact-print still associates this with the llies.

I find this annoying and confusing because I suspected all comments for LIEs to be handled by layoutIE. So I go through hoops to get britanny to process this annotation for llies with the last LIE.

And when I do that the result is this:

import           Test                                     ( abc
                                                          -- comment
                                                               , def 
                                                          )

What? Why is that even a thing? I can understand that it now considers the comment to be a part of the BriDoc, but the annotation's location data should still indicate that the placement should be below the LIE. And why the ,def shifts to the right (or the comment is right associative with respect to the , def which is then forced to the base offet) is even more of a mystery to me. And then we had the comments for the module which shifted to after the where. In that case I agree with ghc-exact-print that the comments belong to the module node. I personally rarely place comments below the element, on occasion to the right of an element, but mostly above an element. And there is no reason not to place them above the element in that context (Since there are no elements there)

So the behaviour of both ghc-exact-print and brittany is far from intuitive to me.

End rant 😛

It isn't really. I want to get a feeling for how to deal with them, since I want to contribute more in the future so I had better learn how to deal with them. I am having difficulty anticipating both brittany and ghc-exact-print and improving bad results with more specific instructions to brittany is also harder than I expect. Let's take your tests as an example: ```haskell #test import-with-comments-2 import Test ( abc , def -- comment ) #test import-with-comments-3 import Test ( abc -- comment ) ``` Both these comments end up on the (Located [LIE]) and not on the LIE for abc and def. This means that ghc-exact-print thinks that the comment belongs to the entire binding list or rather to the ) which semantically means the same thing. It goes even further than that: ```haskell #test import-with-comments-4 import Test ( abc , def -- comment ) ``` I would consider any comments on the right of some element to belong to the element on its left (There might be a case, where there is a more semantically correct element **directly below** the comment), but ghc-exact-print still associates this with the llies. I find this annoying and confusing because I suspected all comments for LIEs to be handled by layoutIE. So I go through hoops to get britanny to process this annotation for llies with the last LIE. And when I do that the result is this: ```haskell import Test ( abc -- comment , def ) ``` What? Why is that even a thing? I can understand that it now considers the comment to be a part of the BriDoc, but the annotation's location data should still indicate that the placement should be below the LIE. And why the `,def` shifts to the right (or the comment is right associative with respect to the `, def` which is then forced to the base offet) is even more of a mystery to me. And then we had the comments for the module which shifted to after the where. In that case I agree with ghc-exact-print that the comments belong to the module node. I personally rarely place comments below the element, on occasion to the right of an element, but mostly above an element. And there is no reason not to place them above the element in that context (Since there are no elements there) So the behaviour of both ghc-exact-print and brittany is far from intuitive to me. End rant :stuck_out_tongue:
sniperrifle2004 commented 2017-12-19 14:32:04 +01:00 (Migrated from github.com)

I figured it out, but those delta positions sure are odd. I am not sure why certain comments are attached to the llies at all, while the delta positions clearly still indicate they are relative to def or abc.

I figured it out, but those delta positions sure are odd. I am not sure why certain comments are attached to the llies at all, while the delta positions clearly still indicate they are relative to def or abc.
lspitzner commented 2017-12-19 15:13:46 +01:00 (Migrated from github.com)

@sniperrifle2004 agreed. See the discussion at https://github.com/alanz/ghc-exactprint/issues/53. I made some suggestions there that I think would be saner approaches, but then I have no much of an idea of how the actual parsing and processing logic for DPs works. (There is also #31 where the strange DPs make things very hard/impossible for brittany to do the correct thing.)

@sniperrifle2004 agreed. See the discussion at https://github.com/alanz/ghc-exactprint/issues/53. I made some suggestions there that I _think_ would be saner approaches, but then I have no much of an idea of how the actual parsing and processing logic for DPs works. (There is also #31 where the strange DPs make things very hard/impossible for brittany to do the correct thing.)
lspitzner commented 2017-12-19 17:58:09 +01:00 (Migrated from github.com)

Ah, and of course new-build treats "--ghc-options" differently. I was so evil to revert that merge, let me fix dev branch first.

Ah, and of course new-build treats "--ghc-options" differently. I was so evil to revert that merge, let me fix `dev` branch first.
tfausak commented 2017-12-20 03:11:52 +01:00 (Migrated from github.com)

@sniperrifle2004 thanks for humoring my alignment-free lifestyle 😄 This PR is looking great!

@sniperrifle2004 thanks for humoring my alignment-free lifestyle 😄 This PR is looking great!
👍 1
lspitzner commented 2017-12-20 15:22:23 +01:00 (Migrated from github.com)

@sniperrifle2004 and you are right,

import           Test                                     ( abc
                                                          -- comment
                                                               , def 
                                                          )

looks really strange, and is probably a bug hiding in the lower layers of brittany. I don't understand how you triggered it however, as the current code does not expose this and still seems to go through the hoop you mentioned. (Btw. other layouters in Expr.hs use the same approach, iirc.)

Do you currently have uncommitted/unpushed stuff? There is a couple of minor fixups I wish to make.

@sniperrifle2004 and you are right, ~~~~.hs import Test ( abc -- comment , def ) ~~~~ looks really strange, and is probably a bug hiding in the lower layers of `brittany`. I don't understand how you triggered it however, as the current code does not expose this and still seems to go through the hoop you mentioned. (Btw. other layouters in `Expr.hs` use the same approach, iirc.) Do you currently have uncommitted/unpushed stuff? There is a couple of minor fixups I wish to make.
sniperrifle2004 commented 2017-12-21 06:01:28 +01:00 (Migrated from github.com)

@lspitzner It was partly because I did not use docWrapNode llies on just the last element at the time. I used it on both abc and , def (The first and the last element). I did not realize how wrong this was until I used dump-annotations. In addition the current code only affects def not the docSeq with the docCommaSep which seems to be significant as well, since just adding docWrapNode llies to abc now results in

( abc
-- comment
, def
)

If I also change the layoutAnnAndSepLLIEs to call docWrapNode on the , def docSeq then I get:

( abc
-- comment
     , def
)

I do not currently have any pending changes (I have been a bit busy).

@lspitzner It was partly because I did not use `docWrapNode llies` on just the last element at the time. I used it on both `abc` and `, def` (The first and the last element). I did not realize how wrong this was until I used `dump-annotations`. In addition the current code only affects `def` not the `docSeq` with the `docCommaSep` which seems to be significant as well, since just adding `docWrapNode llies` to `abc` now results in ```haskell ( abc -- comment , def ) ``` If I also change the `layoutAnnAndSepLLIEs` to call `docWrapNode` on the `, def` docSeq then I get: ```haskell ( abc -- comment , def ) ``` I do not currently have any pending changes (I have been a bit busy).
lspitzner commented 2017-12-21 19:51:07 +01:00 (Migrated from github.com)

@sniperrifle2004 oh, i should probably remark on that last refactor: My main concern was not that the code was bad, but it was not clear to me how the sharing worked (i.e. usage of docShareWrapper etc.). To be fair, sharing is probably irrelevant because there are no arbitrarily nested nodes below in the import/export statements anyways, so the complexity is linear anyways, but it does not hurt to properly implement the sharing either.

@sniperrifle2004 oh, i should probably remark on that last refactor: My main concern was not that the code was bad, but it was not clear to me how the sharing worked (i.e. usage of `docShareWrapper` etc.). To be fair, sharing is probably irrelevant because there are no arbitrarily nested nodes below in the import/export statements anyways, so the complexity is linear anyways, but it does not hurt to properly implement the sharing either.
sniperrifle2004 commented 2017-12-22 07:03:07 +01:00 (Migrated from github.com)

I added a compact layout triggered when the indentPolicy is IndentPolicyLeft.

In addition, I removed the fieldLabel part of the ThingWith. I realized this couldn't be executed at all, since the information to distinguish fields and other functions isn't available yet.

I also added code to handle the case where there are comments in a ThingWith. This also means those lists can now expand over multiple lines. The only case where this is troublesome is the case where a single item with a list would expand resulting in this:

import           Test                                     ( Long( List
                                                                , Of
                                                                , Things
                                                                ) )

I tried several things to get this to look a little better, but nothing quite worked. So it seems this is the price for keeping such a line from (excessively) overflowing the column limit.

@lspitzner I think that is closer to what I intended to do (ie. share the list between alternatives). I wasn't really concerned with whether it had to be shared. I just thought that it would be better if it was.

I added a compact layout triggered when the indentPolicy is IndentPolicyLeft. In addition, I removed the fieldLabel part of the ThingWith. I realized this couldn't be executed at all, since the information to distinguish fields and other functions isn't available yet. I also added code to handle the case where there are comments in a ThingWith. This also means those lists can now expand over multiple lines. The only case where this is troublesome is the case where a single item with a list would expand resulting in this: ```haskell import Test ( Long( List , Of , Things ) ) ``` I tried several things to get this to look a little better, but nothing quite worked. So it seems this is the price for keeping such a line from (excessively) overflowing the column limit. @lspitzner I think that is closer to what I intended to do (ie. share the list between alternatives). I wasn't really concerned with whether it *had* to be shared. I just thought that it would be better if it was.
🎉 1
tfausak (Migrated from github.com) reviewed 2017-12-22 14:44:23 +01:00
src-literatetests/tests-context-free.blt
tfausak (Migrated from github.com) commented 2017-12-22 14:44:23 +01:00

This is actually not context free anymore, right? The indentation here depends on the length of the module name being imported.

This is actually not context free anymore, right? The indentation here depends on the length of the module name being imported.
sniperrifle2004 (Migrated from github.com) reviewed 2017-12-22 15:49:38 +01:00
src-literatetests/tests-context-free.blt
sniperrifle2004 (Migrated from github.com) commented 2017-12-22 15:49:38 +01:00

More on the position of the first binding, which I chose to stick to the import line. So you rather want this:

import MoreThanSufficientlyLongModuleNameWithSome
  (items, that, ...)

An eventually:

import MoreThanSufficientlyLongModuleNameWithSome
  ( items
  , that
  , ...
  )
More on the position of the first binding, which I chose to stick to the import line. So you rather want this: ```haskell import MoreThanSufficientlyLongModuleNameWithSome (items, that, ...) ``` An eventually: ```haskell import MoreThanSufficientlyLongModuleNameWithSome ( items , that , ... ) ```
tfausak (Migrated from github.com) reviewed 2017-12-22 16:19:38 +01:00
src-literatetests/tests-context-free.blt
tfausak (Migrated from github.com) commented 2017-12-22 16:19:38 +01:00

👍 Yup!

:+1: Yup!
lspitzner commented 2017-12-29 17:04:16 +01:00 (Migrated from github.com)

@tfausak can/did you have another look at the last commit?

@sniperrifle2004 i have not forgotten about this; i have been toying a bit with the ThingWith case too, without finding a nice(r) solution yet either. But perhaps it is better to not let ThingWith hold up the PR. I have reviewed the last commits and would like to merge before this gets even larger, unless you have something planned still - have you?

@tfausak can/did you have another look at the last commit? @sniperrifle2004 i have not forgotten about this; i have been toying a bit with the `ThingWith` case too, without finding a nice(r) solution yet either. But perhaps it is better to not let `ThingWith` hold up the PR. I have reviewed the last commits and would like to merge before this gets even larger, unless you have something planned still - have you?
tfausak (Migrated from github.com) reviewed 2017-12-29 17:21:41 +01:00
src-literatetests/tests-context-free.blt
@ -596,0 +745,4 @@
-- Test
import Data.List (nub) -- Test
{- Test -}
import qualified Data.List as L (foldl') {- Test -}
tfausak (Migrated from github.com) commented 2017-12-29 17:21:41 +01:00

These are the only ones that look weird to me. The indentation depends on the length of the Thing identifier. I don't think I've ever actually had an import like this, but I think I'd format it as:

import Test
  ( Thing
    ( With
    -- Comments
    , and
    -- also
    , items
    -- !
    )
  )
These are the only ones that look weird to me. The indentation depends on the length of the `Thing` identifier. I don't think I've ever actually had an import like this, but I think I'd format it as: ``` hs import Test ( Thing ( With -- Comments , and -- also , items -- ! ) ) ```
sniperrifle2004 (Migrated from github.com) reviewed 2018-01-06 12:49:34 +01:00
src-literatetests/tests-context-free.blt
@ -596,0 +745,4 @@
-- Test
import Data.List (nub) -- Test
{- Test -}
import qualified Data.List as L (foldl') {- Test -}
sniperrifle2004 (Migrated from github.com) commented 2018-01-06 12:49:34 +01:00

Ah I should have expected this. I'll take a look.

Ah I should have expected this. I'll take a look.
sniperrifle2004 commented 2018-01-06 12:49:47 +01:00 (Migrated from github.com)

@lspitzner I have nothing else planned really. I do suspect that it might be a good idea to add an option to choose the compact layout independent of the indent policy, but I don't know that for certain. Adding that later is no problem.

Another thing that comes to mind is that the import column is still set to 60. This is probably the best time to change it, since only the tests depend on it.

@lspitzner I have nothing else planned really. I do suspect that it might be a good idea to add an option to choose the compact layout independent of the indent policy, but I don't know that for certain. Adding that later is no problem. Another thing that comes to mind is that the import column is still set to 60. This is probably the best time to change it, since only the tests depend on it.
lspitzner commented 2018-03-11 17:10:56 +01:00 (Migrated from github.com)

For the record, this was not closed intentionally, in fact I did not ever press any such button, and I was not aware that deleting a branch would have this effect. Sorry again for leaving this PR hanging for so long, it is certainly still on my to-do list for the next release.

For the record, this was not closed intentionally, in fact I did not ever press any such button, and I was not aware that deleting a branch would have this effect. Sorry again for leaving this PR hanging for so long, it is certainly still on my to-do list for the next release.

Pull request closed

This pull request cannot be reopened because the branch was deleted.
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#83
There is no content yet.
Delete Branch "import"

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?