Skip to content

Comments

Add new custom hlint rule for runSetting.#2718

Merged
elland merged 2 commits intodevelopfrom
improve-hlint-and-apply
Sep 22, 2022
Merged

Add new custom hlint rule for runSetting.#2718
elland merged 2 commits intodevelopfrom
improve-hlint-and-apply

Conversation

@elland
Copy link
Contributor

@elland elland commented Sep 21, 2022

Also applies hlint again to the whole codebase (excluding tests), as we had some drift between finalising hlint and new PRs being merged without being linted / having CI catch those cases.

I also disabled the pipefail from the script, as that would short-circuit the linter on first issue found. Hopefully that doesn't mess with CI.

PS: This will fail CI linters phase until #2715 has been merged.

Also applies hlint again to the whole codebase (excluding tests), as we
had some drift between finalising hlint and new PRs being merged without
being linted / having CI catch those cases.

I also disalbed the pipefail from the script, as that would
short-circuit the linter on first issue found. Hopefully that doesn't
mess with CI.

PS: This will fail CI linters phase until #2715 has been merged.
@elland elland temporarily deployed to cachix September 21, 2022 08:12 Inactive
@elland elland temporarily deployed to cachix September 21, 2022 08:12 Inactive
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Sep 21, 2022
Copy link
Member

@akshaymankar akshaymankar left a comment

Choose a reason for hiding this comment

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

Does checking only files changed by PR mean that adding new rules won't run over everything in CI? Maybe we should reconsider this approach and just run hlint on everything.

@elland
Copy link
Contributor Author

elland commented Sep 21, 2022

@akshaymankar We might want to do that indeed, at least on CI. For this case, or when I touch the rules, I do run on everything (except tests), but that's a fraught process that requires people to remember to do manual steps, and as you mentioned somewhere else, linting runs parallel to the other CI steps, so it taking 5x or more longer wouldn't impact us, as it would remain much faster than integration tests. I'll bring this up to the rest of the team.

@elland elland temporarily deployed to cachix September 21, 2022 11:19 Inactive
@elland elland temporarily deployed to cachix September 21, 2022 11:19 Inactive
@elland elland merged commit 0102d7e into develop Sep 22, 2022
@elland elland deleted the improve-hlint-and-apply branch September 22, 2022 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants