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

Don't run hlint on testdata directories #3901

Merged
merged 7 commits into from
Jan 16, 2024

Conversation

fendor
Copy link
Collaborator

@fendor fendor commented Dec 14, 2023

closes #3900

@fendor
Copy link
Collaborator Author

fendor commented Dec 14, 2023

@soulomoon Does this look good?

@soulomoon
Copy link
Collaborator

@soulomoon
Copy link
Collaborator

perhaps it is some kind of HLint bug?

@fendor
Copy link
Collaborator Author

fendor commented Dec 15, 2023

Hm, maybe it only accepts the ignore-glob param once?

@fendor
Copy link
Collaborator Author

fendor commented Dec 15, 2023

No, no idea, can't reproduce locally with version 3.6.1.

@soulomoon
Copy link
Collaborator

soulomoon commented Jan 10, 2024

May be we can try to bumbup the version to 3.6.1 ?

@fendor fendor requested a review from michaelpj as a code owner January 10, 2024 16:25
@michaelpj
Copy link
Collaborator

Can we not put this in the .hlint.yaml file or something so it's always applied?

@fendor
Copy link
Collaborator Author

fendor commented Jan 10, 2024

No, ignore-glob is seemingly only recognised by the cli. But on CI, not even that seems to be true.

@fendor
Copy link
Collaborator Author

fendor commented Jan 10, 2024

I did it! It looks like @actions/exec does something funky with cmd arguments that contain quotes. I don't really know what's going, but I am suspecting that a shell is acting up, maybe escaping or resolving *?

@soulomoon
Copy link
Collaborator

soulomoon commented Jan 11, 2024

LGTM, it seems checks for the testdata's files are gone.

@fendor fendor force-pushed the fix/hlint-ignore-testdata branch from 0638bc0 to 927f811 Compare January 11, 2024 08:42
@fendor fendor enabled auto-merge January 11, 2024 08:42
@fendor fendor added the merge me Label to trigger pull request merge label Jan 11, 2024
@michaelpj
Copy link
Collaborator

@fendor fendor removed the merge me Label to trigger pull request merge label Jan 11, 2024
@fendor fendor disabled auto-merge January 11, 2024 10:06
@fendor
Copy link
Collaborator Author

fendor commented Jan 11, 2024

Yeah that looks good, I was looking for a different option, apparently. Will update the PR and delete the respective .hlint.yaml files.

@fendor fendor requested a review from soulomoon as a code owner January 11, 2024 10:08
@michaelpj michaelpj enabled auto-merge (squash) January 11, 2024 10:37
@fendor fendor disabled auto-merge January 11, 2024 10:43
@fendor
Copy link
Collaborator Author

fendor commented Jan 11, 2024

Doesn't seem to be working yet

@soulomoon
Copy link
Collaborator

Since none of us can repro locally, I tend to think there is a Github action CI up stream bug.

@fendor
Copy link
Collaborator Author

fendor commented Jan 12, 2024

Adding

- ignore: { "within": '**/testdata/**' }
- ignore: { "within": '**/test/data/**' }

does not work for me locally either. That may be a hlint bug worth discussing upstream.

@soulomoon
Copy link
Collaborator

soulomoon commented Jan 12, 2024

locally testing, ditto
create a ticket to hlint ndmitchell/hlint#1552

@soulomoon
Copy link
Collaborator

soulomoon commented Jan 12, 2024

I realize, It might be a confliction between --git and --ignore-glob @fendor

@soulomoon
Copy link
Collaborator

I found the source, this pull request ndmitchell/hlint#1552 should fix the ignore-glob problem.

Copy link
Collaborator

@soulomoon soulomoon left a comment

Choose a reason for hiding this comment

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

looking good, the problem should be fixed with it once the hlint support ignore glob with arg --git

@michaelpj
Copy link
Collaborator

This seems semantically correct even if there's a hlint problem, so let's merge it anyway.

@michaelpj michaelpj added the merge me Label to trigger pull request merge label Jan 14, 2024
@mergify mergify bot merged commit ccfc57b into haskell:master Jan 16, 2024
41 checks passed
josephsumabat pushed a commit to josephsumabat/haskell-language-server that referenced this pull request Jan 22, 2024
* Don't run hlint on testdata directories

* Bump hlint version

* Remove quotes

* Ignore test data directories in .hlint.yaml

---------

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Co-authored-by: Michael Peyton Jones <[email protected]>
Co-authored-by: Patrick Wales <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Label to trigger pull request merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CI: Hlint should not lint test data files.
3 participants