Setup hlint to run in the project#3816
Conversation
There have been a smattering of efforts to run `hlint` on the project: * 1b77d7e * afd4605 * 5860f1e * b120af7 * a647c78 * f806a6f * 20f1200 * 0b07beb The last explicit `hlint` inspiried cleanup was three and a half years ago. Over that time, we've accumulated almost 400 hints from `hlint`. It's unclear if these are all new or if they've been around for a while and the previous efforts only addressed some of the issues. To mitigate the number of new hints, we setup `hlint` properly in the project. Given that there are too many hints to attempt to take on all at once, we ignore all of the current hints with a `.hlint.yaml` file provided by the `--default` flag. We can triage the hints that we think are useful, and ignore the ones we don't find useful. The motivation behind doing this change now is that we want to be more consistent with the language extensions in use: purescript#3805. Since `hlint` can enforce the use of language extensions: https://github.com/ndmitchell/hlint/tree/9f1e23655c4fac537446a2600dd2f5d89825406c#restricting-items, it seemed like an alright approach to solving this problem: purescript#3805 (comment). We'll move on to cleaning up language extensions after we've decided what to do about this approach.
Turns out `make` doesn't exist on the Windows environment of TravisCI. It has `cmake` instead. Also, the macOS environment of TravisCI doesn't have the same version of `mktemp` or `rm` that exists on the Linux environment. It is probably using the BSD versions of those while we expected the GNU versions. We write things more agnostic by taking in the build and bin directories. This way, we can call the script with arguments instead of expecting it to know more information than it should. It was dubious of us to assume the path to `bin` would always work anyway. To address the Windows issue, we call the script directly to install `hlint` and run `hlint` manually once it's installed. This isn't ideal because it means that we have two workflows: one using `make` the other orchestrating things by hand. Hopefully, we can address these issues better in the future.
Looks like each line in this YAML string is run as its own shell process. So maybe the variables aren't available directly to the script? It's unclear why this didn't work but a few lines above we had `URL` available in the environment. In any case, we try putting it all on one line so it should hopefully be part of the environment the script has available.
There was a problem hiding this comment.
Requesting a review from @hdgarrood as he tends to do most TravisCI stuff. But anybody should feel free to review. Also, feel free to re-assign.
| export CI_RELEASE=true | ||
| fi | ||
| script: | ||
| - hlint --git |
There was a problem hiding this comment.
I was unsure how to reconcile this step with the timeout stuff happening below. Should this be part of the same timeout? Should this be in its own timeout? Should the ci/build.sh script call out to this?
There was a problem hiding this comment.
I think having the ci/build.sh script call out to hlint would be my preference. I suspect it doesn't really matter whether it's included in the timeout or not, because I don't suspect running hlint takes very long, but I think it's preferable to only have to look in one place to see what each CI job is actually checking.
There was a problem hiding this comment.
What does the --git flag do? I haven't managed to find documentation for it.
There was a problem hiding this comment.
Sounds good.
--git finds only the tracked files and lints them.
| BIN_DIR=$(bin_dir) BUILD_DIR=$(build_dir) $< | ||
| touch $@ | ||
|
|
||
| clean: ## Remove build artifacts |
There was a problem hiding this comment.
Is this intended to cover all of the places that a build creates files? I know there are many others, most of which I had to clean while switching the polykinds PR on and off: .test_modules, **/.stack-work, tests/support/node_modules, tests/support/bower_components, etc. A git clean -fdX takes care of all of this; what value is make clean intended to add?
There was a problem hiding this comment.
Is this intended to cover all of the places that a build creates files?
Yes, ideally.
A
git clean -fdXtakes care of all of this; what value ismake cleanintended to add?
The whole purpose of a build system (make or otherwise) is to not have to remember a bunch of different switches and flags and the order in which to call them for day-to-day tasks.
There was a problem hiding this comment.
In that case, all I'm trying to say is that I think you're missing a bunch of paths. Here's the output of git clean -ndX on a recent build for me:
Would remove .psci_modules/
Would remove .stack-work/
Would remove .test_modules/
Would remove lib/purescript-ast/.stack-work/
Would remove lib/purescript-ast/purescript-ast.cabal
Would remove lib/purescript-cst/.stack-work/
Would remove lib/purescript-cst/purescript-cst.cabal
Would remove purescript.cabal
Would remove tests/purs/docs/output/
Would remove tests/purs/make/
Would remove tests/purs/publish/basic-example/output/
Would remove tests/support/bower_components/
Would remove tests/support/node_modules/
Would remove tests/support/package-lock.json
Would remove tests/support/pscide/output/
I haven't done a checkout of this PR to test, though, so maybe some/all of these have been moved into .build? I don't know. Just trying to make sure make clean works as intended.
There was a problem hiding this comment.
Right, it sounds like adding a clean target in the Makefile whose implementation is git clean -fdX is probably the way to go here.
There was a problem hiding this comment.
Sorry, I wasn't intending to have this rule fix all of the problems with the current build system. I only meant to clean up the new artifacts introduced with this PR.
Given that this is generating enough discussion by itself, and the build system hasn't been cleaning up after itself up until this point, I'm going to split this out to its own PR so we can focus on getting the hlint changes reviewed.
hdgarrood
left a comment
hdgarrood
left a comment
There was a problem hiding this comment.
This looks good to me, assuming the clean changes are going to be split out and addressed separately.
|
Thanks @joneshf! |
|
Sorry, this wasn't quite ready to merge. We hadn't addressed @hdgarrood's suggestion or @rhendric's concern. To mitigate this situation in the future, how do we better communicate that a PR still has work to do? |
|
There's a "needs more work" label that is sometimes used, I think that'd do it. |
|
Got it. Thanks! |
|
I guess if I hadn't approved prematurely it probably wouldn't have happened. |
|
Sorry, should this be reverted? I merged it because it was approved and sat for several weeks. |
|
I don't think it's worth reverting, the unresolved suggestions don't seem that important to me. |
5 participants
As the first step in cleaning up language extensions, we're setting up
hlint. Assuming this approach works, we'll then use hlint to restrict which language extensions can be used.