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

Use HLint. #1045

Merged
merged 1 commit into from
Nov 4, 2024
Merged

Use HLint. #1045

merged 1 commit into from
Nov 4, 2024

Conversation

chungyc
Copy link
Contributor

@chungyc chungyc commented Sep 24, 2024

I thought it would be nice to have a linter, so this adds an HLint workflow which uploads its results to GitHub code scanning. If you do not want a linter or prefer other ways to do linting, feel free to close this pull request without merging.

This change disables hints that would trigger for the existing code. Depending on preference, some of them can be enabled later, while others could be permanently disabled. E.g., removing a needless $ seems a good idea, while I am not a fan of the suggestion to use <&> with Hakyll code.

To see what warnings and suggestions may look like, see https://github.com/haskell-actions/hlint-scan#examples.

@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

@LaurentRDC
Copy link
Collaborator

What would be the advantages/disadvantages of doing this?

The one thing I do not want is to have some CI job that can fail if linting fails. As long as this job only provides suggestions, then I'm neutral on this.

@chungyc
Copy link
Contributor Author

chungyc commented Oct 29, 2024

The advantages of using HLint is that it can suggest improvements to code that may be more complicated or expensive than needed. For example, it's helped me catch silly code such as map f (map g l), where map (f . g) l would have been better, or cases where the same module is imported multiple times.

The advantages of using the hlint-scan action for this is that this allows GitHub's code scanning infrastructure to be used to track extant code issues and code issues in pull requests (examples for what they look like are linked in the initial comment for the pull request). Settings for code scanning determine whether these will be considered failed checks. By default, only the most severe errors are considered to be failures, and I don't think any of HLint's built-in hints are set up to be errors; they are at most warnings.

If this were to be used as a status check in the future (which this won't be unless certain settings are changed later), then one thing using GitHub's code scanning infrastructure gives us is that only issues in the code modified by a pull request will be considered an issue for the pull request, and existing issues in other parts of the code base will not be considered an issue for the pull request.

All the HLint hints disabled by this pull request are triggered by the existing Hakyll code, even though I think some of them should be enabled at some point (e.g., avoiding the gratuitous use of $ or the import of the same module multiple times).

@jaspervdj
Copy link
Owner

I don't really have a strong opinion on this. I feel like common sense and empathy with the reader of the code often lead to better style than rule-based tools like HLint. But as long as this doesn't block PRs, I don't really mind having it either.

@LaurentRDC
Copy link
Collaborator

Let's merge this in. Contributors can use HLint's recommendations, or not.

Thanks @chungyc !

@LaurentRDC LaurentRDC merged commit 2bf8b80 into jaspervdj:master Nov 4, 2024
29 checks passed
LaurentRDC added a commit that referenced this pull request Nov 4, 2024
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