Hlint: A handful of fixes to hints#3259
Conversation
0980148 to
ebb58ee
Compare
| let x = | ||
| [toKey' "diagnosticsOn" A..= schemaEntry "diagnostics" | configHasDiagnostics] | ||
| <> nub (mconcat (handlersToGenericSchema <$> handlers)) | ||
| <> Data.List.Extra.nubOrd (mconcat (handlersToGenericSchema <$> handlers)) |
There was a problem hiding this comment.
e49a38d to
9f22ed6
Compare
| scopes = case foldMap mapRef refs of | ||
| Just xs -> xs | ||
| Nothing -> [] | ||
| scopes = Data.Maybe.fromMaybe [] (foldMap mapRef refs) |
There was a problem hiding this comment.
FWIW I personally disagree with this hlint rule pretty strongly. If anything I tend to replace fromMaybe and friends with explicit pattern matches unless there's something higher-order going on!
There was a problem hiding this comment.
Main issue I think that needs to be addressed is the difference between what is configured in .hlint.yaml and the codebase. I myself am trying to be not opinionated in this PR. But I'm also not sure regarding defaults and capabilities of different hlint versions - and how hint levels are dealt with by different tools (hls, the github action in this repository and together with apply-refact). Currently I'm still looking into it.
E.g. for this specific file with hlint version 3.3.6 I see these hints (where this one is a suggestion):
hlint hie-compat/src-ghc86/Compat/HieDebug.hs
hie-compat/src-ghc86/Compat/HieDebug.hs:1:1: Warning: Use module export list
Found:
module Compat.HieDebug where
Perhaps:
module Compat.HieDebug (
module Compat.HieDebug
) where
Note: an explicit list is usually better
hie-compat/src-ghc86/Compat/HieDebug.hs:4:1-35: Warning: Unused LANGUAGE pragma
Found:
{-# LANGUAGE StandaloneDeriving #-}
Perhaps you should remove it.
hie-compat/src-ghc86/Compat/HieDebug.hs:(132,18)-(134,23): Suggestion: Replace case with fromMaybe
Found:
case foldMap mapRef refs of
Just xs -> xs
Nothing -> []
Perhaps:
Data.Maybe.fromMaybe [] (foldMap mapRef refs)
3 hintsChanged this in the hlint config: 5a68e9f
There was a problem hiding this comment.
We can absolutely continue to tweak our hlint config, and I think turning off a rule is equally valid to fixing it if we don't think the rule clearly adds value.
a6499b8 to
df2e1a4
Compare
|
In terms of getting our codebase clean: we have some I'm also very pro getting rid of warnings and pruning any rules that don't add value 👍 |
kokobd
left a comment
There was a problem hiding this comment.
modification to hls-code-range-plugin looks good to me.
drsooch
left a comment
There was a problem hiding this comment.
Lgtm on number formatting plugin
6a3786b to
1ec2fea
Compare
Goal
Less existing warnings in pull requests - by addressing some of the issues.
Changes
rwe/actions-hlint-run@v2Review
While most of the changes seem to be trivial (whitespace or parens removed), some changes prefer
extrafunctions (as it has been configured). What I think is potentially critical is removing extensions as suggested by hlint: Is it possible that one of the removed extensions is actually changing functionality? If so, please comment.