Use pre-commit for better linting #2492
Conversation
Fixes these warnings:
error: function `assert_points_eq` is never used
--> src/utils/test.rs:72:8
|
72 | pub fn assert_points_eq(actual: Vec<Point>, expected_ascii: &str) {
| ^^^^^^^^^^^^^^^^
|
= note: `-D dead-code` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(dead_code)]`
error: function `points_to_ascii` is never used
--> src/utils/test.rs:89:8
|
89 | pub fn points_to_ascii(points: Vec<Point>) -> String {
| ^^^^^^^^^^^^^^^
.github/ISSUE_TEMPLATE/bug_report.md
Outdated
| @@ -8,28 +8,36 @@ assignees: '' | |||
|
|
|||
| <!--- NOTE: PLEASE FILL OUT TEMPLATE RATHER THAN DELETING ---> | |||
|
|
|||
| **Describe the bug** | |||
| ## Describe the bug | |||
There was a problem hiding this comment.
I'm not sure about this change. It converts the text to a header instead of bold which imho is better. Its a nit but the change seems unrelated unless im missing something.
I guess this was a lint? Maybe we can disable it?
There was a problem hiding this comment.
Yes it was a lint: https://github.com/markdownlint/markdownlint/blob/main/docs/RULES.md#md036---emphasis-used-instead-of-a-header
Why do think bold is better? Do you prefer how it's displayed? Semantically I think the lint is right here. We could use a lower heading level though, so that it looks similar to bold text.
There was a problem hiding this comment.
Changed it to h4 heading. Please let me know what you think.
|
I like most of these changes and agree that ensuring all of the lints are run is a good goal. However I'm nervous about the precommit hook. In the past I've worked on projects with them that can be more annoying than helpful especially when there are other places that can do the same check. Personally, I dislike changes that get in the way of people making contributions and playing around with the codebase. As is the CI can always run the checks and we can catch the issues in review. But thats just my opinion. Would be good to hear from others like @fredizzimo |
Well, the pre-commit hook is opt-in (by running Just to avoid misunderstandings, pre-commit on CI works just like a regular linter. It will not require the contributors to retroactively fix old commits in their PR or something, it just checks that the latest PR state is clean. Personally, I don't want to think about code formatting, style, etc. This is why I find the pre-commit hooks super convenient, because they fix most of the stuff automatically for me, and warn me if manual intenvention is needed. Nothing is worse than submitting a large PR and then have to fix a bunch of code style nits by hand because CI complains :) |
|
By the way, you might want to consider to use https://pre-commit.ci/ instead of the GitHub Actions workflow, then CI can commit code style fixes automatically (where possible) so that users that don't use pre-commit locally don't have to clean up code smell themselves and don't need additional work. Downside is that these cleanup commits don't play well with |
Test Results 6 files ± 0 6 suites ±0 19s ⏱️ -1s Results for commit 590385a. ± Comparison against base commit 12a415a. This pull request removes 2 tests. |
|
So you want me to rebase, or is this undesired? Then I'd close this. |
This PR introduces a configuration for the pre-commit framework to establish a linter setup that is used both on CI and on the dev's machine when comitting changes. This also introduces some checks such as clippy, codespell and gitleaks.
This also cleans up the code so that it matches the linter configuration to prevent the situation that contributors have to fix unrelated code to makes the PR checks pass.