jaspervdj / hakyll

A static website compiler library in Haskell
jaspervdj.be/hakyll
Other
2.7k stars 409 forks source link

Use HLint. #1045

Closed chungyc closed 2 weeks ago

chungyc commented 1 month ago

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[bot] commented 1 month ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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

Thanks @chungyc !