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

Fix some hlint suggestions #132

Merged
sergv merged 11 commits from master into master 2018-04-04 06:50:49 +02:00
Conversation 8 Commits 11 Files Changed 15
sergv commented 2018-03-29 23:24:01 +02:00 (Migrated from github.com)
There is no content yet.
lspitzner commented 2018-03-30 00:23:43 +02:00 (Migrated from github.com)

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.

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 `do`s: 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 commented 2018-03-30 00:37:31 +02:00 (Migrated from github.com)

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?

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: ~~~~.hs 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 ~~~~.hs myDoc = docAltFilter [ (,) someCondition $ some large expression , (,) otherwise $ other large expression ] ~~~~ which is rather annoying to read, we could write something like ~~~~.hs 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 commented 2018-03-30 02:04:34 +02:00 (Migrated from github.com)

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.

Thanks for prompt repsonse! The `hlint` is pretty configurable - each suggestion can be disabled individually and config can be added into the repository. Regarding `do`s 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 commented 2018-03-30 12:32:02 +02:00 (Migrated from github.com)

@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 Returning to your example for replacement of `docAltFilter`. What do you think about using operators to tie condidion and expression? E.g. instead of ```hs myDoc = runFilteredAlternative $ do addAlternativeCond someCond $ some large expression addAlternative $ other large expression ``` write something like ```hs myDoc = runFilteredAlternative $ do someCond |-> some large expression otherwise |-> other large expression ```
lspitzner commented 2018-03-30 16:40:02 +02:00 (Migrated from github.com)

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 :-)

> 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 > > ~~~~.hs > 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 commented 2018-04-02 23:56:15 +02:00 (Migrated from github.com)

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.

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.
lspitzner (Migrated from github.com) reviewed 2018-04-03 20:33:58 +02:00
lspitzner (Migrated from github.com) left a comment

Thanks, i see you refactored things is Decl.hs some more and you even updated some commented-out stuff. I think this really helps readability.

I have commented on some few things i noticed. Really nothing serious, but if you don't mind one last iteration.

Thanks, i see you refactored things is Decl.hs some more and you even updated some commented-out stuff. I think this really helps readability. I have commented on some few things i noticed. Really nothing serious, but if you don't mind one last iteration.
src/Language/Haskell/Brittany/Internal/LayouterBasics.hs
@ -415,8 +422,20 @@ docExt x anns shouldAddComment = allocateNode $ BDFExternal
docAlt :: [ToBriDocM BriDocNumbered] -> ToBriDocM BriDocNumbered
lspitzner (Migrated from github.com) commented 2018-04-03 20:08:31 +02:00

I don't really see the usecase for this, and one might expect the same pattern as with when/whenM:

when :: Applicative f => Bool -> f () -> f ()
whenM :: Monad m => m Bool -> m () -> m ()

(Also it is not used anywhere afaict.)

I don't really see the usecase for this, and one might expect the same pattern as with `when/whenM`: ~~~~.hs when :: Applicative f => Bool -> f () -> f () whenM :: Monad m => m Bool -> m () -> m () ~~~~ (Also it is not used anywhere afaict.)
src/Language/Haskell/Brittany/Internal/Layouters/Decl.hs
@ -309,2 +309,4 @@
(docForceSingleline . return <$> gs)
)
wherePart = case mWhereDocs of
Nothing -> Just docEmpty
lspitzner (Migrated from github.com) commented 2018-04-03 20:10:39 +02:00

could you move this let up in group with the previous let-binds?

could you move this let up in group with the previous let-binds?
lspitzner (Migrated from github.com) commented 2018-04-03 20:22:44 +02:00

add comment

return () -- no alternatives exclusively when `length clauseDocs /= 1`
add comment ~~~~.hs return () -- no alternatives exclusively when `length clauseDocs /= 1` ~~~~
src/Language/Haskell/Brittany/Internal/Layouters/IE.hs
@ -121,25 +118,22 @@ layoutLLIEs :: Bool -> Located [LIE RdrName] -> ToBriDocM BriDocNumbered
layoutLLIEs enableSingleline llies = do
lspitzner (Migrated from github.com) commented 2018-04-03 20:18:18 +02:00

Can factor out runFilteredAlternative $ out of the cases.

Can factor out `runFilteredAlternative $` out of the `case`s.
sergv (Migrated from github.com) reviewed 2018-04-03 23:36:39 +02:00
src/Language/Haskell/Brittany/Internal/LayouterBasics.hs
@ -415,8 +422,20 @@ docExt x anns shouldAddComment = allocateNode $ BDFExternal
docAlt :: [ToBriDocM BriDocNumbered] -> ToBriDocM BriDocNumbered
sergv (Migrated from github.com) commented 2018-04-03 23:36:38 +02:00

Indeed it's not used anywhere. I think I tried to use it somewhere but found a way without it.

Indeed it's not used anywhere. I think I tried to use it somewhere but found a way without it.
sergv (Migrated from github.com) reviewed 2018-04-04 00:32:48 +02:00
src/Language/Haskell/Brittany/Internal/Layouters/Decl.hs
@ -309,2 +309,4 @@
(docForceSingleline . return <$> gs)
)
wherePart = case mWhereDocs of
Nothing -> Just docEmpty
sergv (Migrated from github.com) commented 2018-04-04 00:32:48 +02:00

Done.

Done.
sergv (Migrated from github.com) reviewed 2018-04-04 00:32:53 +02:00
src/Language/Haskell/Brittany/Internal/Layouters/IE.hs
@ -121,25 +118,22 @@ layoutLLIEs :: Bool -> Located [LIE RdrName] -> ToBriDocM BriDocNumbered
layoutLLIEs enableSingleline llies = do
sergv (Migrated from github.com) commented 2018-04-04 00:32:53 +02:00

Done.

Done.
sergv (Migrated from github.com) reviewed 2018-04-04 00:32:59 +02:00
src/Language/Haskell/Brittany/Internal/Layouters/Decl.hs
@ -309,2 +309,4 @@
(docForceSingleline . return <$> gs)
)
wherePart = case mWhereDocs of
Nothing -> Just docEmpty
sergv (Migrated from github.com) commented 2018-04-04 00:32:59 +02:00

Done.

Done.
sergv commented 2018-04-04 00:33:21 +02:00 (Migrated from github.com)

I have addressed your suggestions. Please review.

I have addressed your suggestions. Please review.
lspitzner commented 2018-04-04 06:52:04 +02:00 (Migrated from github.com)

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

Thanks! I'd never have noticed that cabal file duplication :-)
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#132
There is no content yet.
Delete Branch "master"

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?