Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hlint: A handful of fixes to hints #3259

Merged
merged 6 commits into from Oct 10, 2022

Conversation

andys8
Copy link
Collaborator

@andys8 andys8 commented Oct 7, 2022

Goal

Less existing warnings in pull requests - by addressing some of the issues.

Changes

Review

While most of the changes seem to be trivial (whitespace or parens removed), some changes prefer extra functions (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.

@andys8 andys8 force-pushed the improvement/hlint-fixes-extensions branch 2 times, most recently from 0980148 to ebb58ee Compare October 7, 2022 14:11
@@ -106,7 +106,7 @@ pluginsToVSCodeExtensionSchema IdePlugins {..} = A.object $ mconcat $ singlePlug
genericSchema =
let x =
[toKey' "diagnosticsOn" A..= schemaEntry "diagnostics" | configHasDiagnostics]
<> nub (mconcat (handlersToGenericSchema <$> handlers))
<> Data.List.Extra.nubOrd (mconcat (handlersToGenericSchema <$> handlers))
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andys8 andys8 force-pushed the improvement/hlint-fixes-extensions branch 2 times, most recently from e49a38d to 9f22ed6 Compare October 7, 2022 15:02
scopes = case foldMap mapRef refs of
Just xs -> xs
Nothing -> []
scopes = Data.Maybe.fromMaybe [] (foldMap mapRef refs)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Collaborator Author

@andys8 andys8 Oct 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 hints

Changed this in the hlint config: 5a68e9f

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@andys8 andys8 force-pushed the improvement/hlint-fixes-extensions branch 2 times, most recently from a6499b8 to df2e1a4 Compare October 7, 2022 15:34
@michaelpj
Copy link
Collaborator

In terms of getting our codebase clean: we have some hlint rules set to error, and those will fail the build, and hence they have a bunch of exceptions. Reducing the number of exceptions would be great: in that way the "error" rules are the ones we're really saying we don't want to happen ever.

I'm also very pro getting rid of warnings and pruning any rules that don't add value 👍

@andys8 andys8 changed the title (Draft) Bunch of hlint refactorings Hlint: A handful of fixes to hints Oct 7, 2022
@andys8 andys8 marked this pull request as ready for review October 7, 2022 15:45
Copy link
Collaborator

@kokobd kokobd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

modification to hls-code-range-plugin looks good to me.

Copy link
Collaborator

@drsooch drsooch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm on number formatting plugin

@andys8 andys8 force-pushed the improvement/hlint-fixes-extensions branch from 6a3786b to 1ec2fea Compare October 9, 2022 09:21
@michaelpj michaelpj enabled auto-merge (squash) October 9, 2022 17:03
@michaelpj michaelpj merged commit 21c8e9b into haskell:master Oct 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants