Fix some hlint suggestions

[lspitzner]

How configurable is hlint?

There are some good changes in there, but many of these changes that save a few characters feel rather code-golfy and not worth the effort in my opinion. Take the removal of redundant dos: For many of those cases, if the relevant code gets changed in the future, it is very likely that the do will become non-redundant. And you gain nothing from saving a whitespace and two characters.

I approve of anything that has to do with alignment, as that has a significant effect on readability.

For other stuff I really don't see the point in even reviewing the individual cases. Sorry.

[lspitzner]

I'd like to also give an example of something that would be rather useful to improve in regards to general readability of the layouter's code. This function:

docAltFilter :: [(Bool, ToBriDocM BriDocNumbered)] -> ToBriDocM BriDocNumbered
docAltFilter = docAlt . map snd . filter fst

is used often, but the argument being a list of tuples produces ugly code/formatting wherever it is used. There is a different approach that was also mentioned in one reddit/r/haskell discussion recently. What if instead of

myDoc = docAltFilter
  [ (,) someCondition
  $ some
    large
    expression
  , (,) otherwise
  $ other
    large
    expression
  ]

which is rather annoying to read, we could write something like

myDoc = runFilteredAlternative $ do
  addAlternativeCond someCond $ some
    large
    expression
  addAlternative $ other
    large
    expression

This seems to much clearer syntactically, with the downside of having a bit more mental overhead (because we use yet another Monad). But I think that trade-off is worth it, especially with speaking names runFilteredAlternative and addAlternativeCond (and not just runWriter - it is mostly a Writer, right?).

Maybe you could explore this a bit?

[sergv]

Thanks for prompt repsonse! The hlint is pretty configurable - each suggestion can be disabled individually and config can be added into the repository.

Regarding dos I'd agree as they're pretty harmless. Yet, on the other hand, redundant parens, $s or replacement of fmap f $ by f <$> does feel like a good thing - the less operators the better. I do understand that there's quite a few changes and it's tiresome to review them one by one. But on the other hand some changes help to bring constistency to the code, which IMO does improve readability further.

Regarding docAltFilter I did notice this pattern and it strikes me as unwieldy. Let me try and see whether I can come up with a nice monad like you're proposing. It indeed does sound like a simple Writer monad at its core.

[sergv]

@lspitzner Returning to your example for replacement of docAltFilter. What do you think about using operators to tie condidion and expression? E.g. instead of

myDoc = runFilteredAlternative $ do
  addAlternativeCond someCond $ some
    large
    expression
  addAlternative $ other
    large
    expression

write something like

myDoc = runFilteredAlternative $ do
  someCond  |-> some
    large
    expression
  otherwise |-> other
    large
    expression

[lspitzner]

Thanks for prompt repsonse! The hlint is pretty configurable - each suggestion can be disabled individually and config can be added into the repository.

Could you disable the redundant-do part then? I think all other hlint changes I am fine with.

write something like

myDoc = runFilteredAlternative $ do
 someCond  |-> some
   large
   expression
 otherwise |-> other
   large
   expression

It looks cool for sure, however: This particular operator looks a lot like case/guard syntax, where only ever one of the alternatives gets selected - but that is not the case here. And having to use otherwise feels like a downside too, when we really mean to express "always". I am not entirely convinced yet :-)

[sergv]

I have split initial commit into single commit for each hint, added hlint config that disables Redundant do and a couple of other suggestions and replaced docAltFilter with runFilteredAlternative as suggested.

The Semigroup commit will be useful for upgrade to ghc 8.4.

[sergv]

I have addressed your suggestions. Please review.

[lspitzner]

Thanks! I'd never have noticed that cabal file duplication :-)