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

update hlint to 3.8 and prevent linting on testdata dir #4018

Merged
merged 8 commits into from
Jan 26, 2024

Conversation

soulomoon
Copy link
Collaborator

@soulomoon soulomoon commented Jan 26, 2024

This is a following up of #3901

I dig a bit into hlint.

  1. We can only ignore-glob in commandline. config file missing the options for ignore glob.
  2. hlint 3.8 can ignore files more correctly.

@soulomoon soulomoon changed the title update hlint to 3.8 update hlint to 3.8 and fix linting on testdata dir Jan 26, 2024
@soulomoon soulomoon changed the title update hlint to 3.8 and fix linting on testdata dir update hlint to 3.8 and prevent linting on testdata dir Jan 26, 2024
@soulomoon

This comment was marked as resolved.

@soulomoon soulomoon marked this pull request as ready for review January 26, 2024 13:44

- name: 'Checking code'
uses: rwe/actions-hlint-run@v2
with:
hlint-bin: "hlint --with-group=extra"
hlint-bin: "hlint --with-group=extra --ignore-glob=**/testdata/** --ignore-glob=**/test/data/**"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you leave a comment with a link to the hlint issue so we know to move this back into the config file when it gets fixed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I dig into the hlint source code and found this. There is no option to configure the ignore glob, 0 0. Also no issue in hlint about this.

        allowFields v ["lhs","rhs","note","name","side"]
        let hintRuleScope = mempty
        pure $
          Left HintRule {hintRuleSeverity = severity, hintRuleLHS = extendInstances lhs, hintRuleRHS = extendInstances rhs, ..}
            : [Right $ Classify severity hintRuleName "" "" | not isBuiltin]
     else do
        names <- parseFieldOpt "name" v >>= maybe (pure []) parseArrayString
        within <- parseFieldOpt "within" v >>= maybe (pure [("","")]) (parseArray >=> concatMapM parseWithin)
        pure [Right $ Classify severity n a b | (a,b) <- within, n <- ["" | null names] ++ names]

Copy link
Collaborator Author

@soulomoon soulomoon Jan 26, 2024

Choose a reason for hiding this comment

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

correction: within field is just about module by module

Copy link
Collaborator Author

@soulomoon soulomoon Jan 26, 2024

Choose a reason for hiding this comment

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

The only source code I see about using ignoring glob is

        ignore = (toPredicate $ cmdIgnoreGlob cmd)
        toPredicate :: [FilePattern] -> FilePath -> Bool
        toPredicate [] = const False
        toPredicate globs = \x -> not $ null $ m [((), cleanup x)]
            where m = matchMany (map ((),) globs)

where cmdIgnoreGlob is only picked up by commandline arg

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean, didn't you do ndmitchell/hlint#1554 ? So we can fix put this back once that is released?

Copy link
Collaborator Author

@soulomoon soulomoon Jan 26, 2024

Choose a reason for hiding this comment

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

Oh that one is about apply the ignoring glob correctly, it is already included in the hlint 3.8 realease.
Before that, even with commandline ignoring glob, we won't be ignoring some files.
I thought that one would fix config file problem, but I was wrong.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ndmitchell/hlint#1554 is the reason we need to bump up to 3.8

@soulomoon soulomoon enabled auto-merge (squash) January 26, 2024 14:03
@soulomoon soulomoon enabled auto-merge (squash) January 26, 2024 14:42
@soulomoon soulomoon merged commit 4788bfc into haskell:master Jan 26, 2024
40 checks passed
@soulomoon soulomoon deleted the switchtohlint38 branch January 26, 2024 15:26
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.

3 participants