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

Lint code in PRs and when committing to master #6790

Merged
merged 4 commits into from Nov 7, 2020
Merged

Lint code in PRs and when committing to master #6790

merged 4 commits into from Nov 7, 2020

Conversation

tarleb
Copy link
Collaborator

@tarleb tarleb commented Oct 31, 2020

This adds a new GitHub Actions workflow which checks the code with HLint for all PRs and commits to master. Most linter annotations in the code are resolved; the remaining warnings are disabled via configs.

@tarleb
Copy link
Collaborator Author

tarleb commented Oct 31, 2020

This is a bit of an experiment. I have generally had good experiences using linters in this way in other projects. But there is always the danger that it gets in the way more than it helps.

@tarleb tarleb closed this Oct 31, 2020
@tarleb tarleb reopened this Oct 31, 2020
@jgm
Copy link
Owner

jgm commented Nov 1, 2020

Yeah, I'm not sure. I normally don't always apply hlint's suggestions, so I'm not sure I'd want to make it a requirement for commits.

Sometimes I find that some parentheses that aren't strictly necessary are helpful to make the code more readable by humans, for example.

@tarleb
Copy link
Collaborator Author

tarleb commented Nov 1, 2020

That's very true. The PR disables multiple hints because they would have made things worse (IMHO).

I am using automatic linting checks in hslua; so far the experience has been good. However, my feeling is that unnecessary warnings happen more often than in other languages. Maybe this would get annoying quickly when used in a project of pandoc's size.

I haven't tried it yet, but maybe the hint-man bot could be a good alternative.

@mb21
Copy link
Collaborator

mb21 commented Nov 2, 2020

Not sure if this has already been discussed, but we could also go full blown with https://github.com/tweag/ormolu

@tarleb
Copy link
Collaborator Author

tarleb commented Nov 2, 2020

Ormolu and similar tools are great in a company environment IMHO. I wouldn't want to use them in pandoc though.

@jgm jgm merged commit 527346c into jgm:master Nov 7, 2020
@jgm
Copy link
Owner

jgm commented Nov 7, 2020

Let's try it. If the automatic lint action gets to be a pain, we can disable it. But even if we do, these changes to hlint config and to the code are useful.

@tarleb tarleb deleted the lint branch November 7, 2020 21:45
@jgm
Copy link
Owner

jgm commented Nov 15, 2020

@tarleb I'm confused about how this works! (Background: I was trying to add a matching lint step to my pre-push procedure.)
Can you explain it?
The main action is

curl -sSL "${hlint_script}" | sh -s .

where hlint_script = https://raw.github.com/ndmitchell/hlint/master/misc/run.sh.
Now if I got to that URL, I see

#!/bin/sh
curl -sSL https://raw.github.com/ndmitchell/neil/master/misc/run.sh | sh -s -- hlint $*

So, we download a script and run it, and the script says to download itself and pipe itself to hlint? What is going on here?
I tried doing hlint . in the pandoc source directory, but I got many more warnings than the CI gives.

@tarleb
Copy link
Collaborator Author

tarleb commented Nov 15, 2020

I took that line from the hlint docs' CI section. Following all URL redirects and indirections, I end up at https://raw.githubusercontent.com/ndmitchell/neil/master/misc/run.sh, which downloads hlint from its GitHub release page. This does seem overly convoluted.

I tried doing hlint . in the pandoc source directory, but I got many more warnings than the CI gives.

That's weird, I'm not getting any warnings right now. I don't know what's going on there.

@tarleb
Copy link
Collaborator Author

tarleb commented Nov 15, 2020

FWIW, I'm running hlint 3.2.

@tarleb
Copy link
Collaborator Author

tarleb commented Nov 15, 2020

My pre-commit hook is

#!/bin/sh
git diff --cached --name-only | grep '.hs$' | xargs hlint --hint .hlint.yaml

@jgm
Copy link
Owner

jgm commented Nov 16, 2020

I've got an older hlint, that may be the issue.

@jgm
Copy link
Owner

jgm commented Nov 16, 2020

Upgraded hlint. That did it. Thanks for the pre-commit hook.

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

3 participants