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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable pre-commit to use cargo-spellcheck #203

Closed
wants to merge 2 commits into from
Closed

Conversation

hunger
Copy link
Contributor

@hunger hunger commented Oct 10, 2021

pre-commit is a rather convenient way to manage git hooks.

This PR enables to use cargo-spellcheck from pre-commit. I added some information on how to do so into README.md.

  • 馃 Feature

Notes to reviewer:

pre-commit will unfortunately fail building cargo-spellcheck at this time:

       Compiling nlprule-build v0.6.4
       Compiling cargo-spellcheck v0.8.14-alpha.0 (/home/dev/.cache/pre-commit/reponz1unczh)
    error: field is never read: `spans`
      --> src/reflow/iter.rs:22:5
       |
    22 |     spans: IndexMap<Range, Span>,
       |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
       |
    note: the lint level is defined here
      --> src/main.rs:1:9
       |
    1  | #![deny(dead_code)]
       |         ^^^^^^^^^
    
    error: failed to compile `cargo-spellcheck v0.8.14-alpha.0 (/home/dev/.cache/pre-commit/reponz1unczh)`, intermediate artifacts can be found at `/home/dev/.cache/pre-commit/reponz1unczh/target`
    
    Caused by:
      could not compile `cargo-spellcheck` due to previous error
    
Check the log at /home/dev/.cache/pre-commit/pre-commit.log

But this should be unrelated to this PR.

馃摐 Checklist

  • [-] Works on the ./demo sub directory
  • [-] Test coverage is excellent and passes
  • [-] Documentation is thorough

None of these applies I think.

@drahnr
Copy link
Owner

drahnr commented Oct 11, 2021

Hey @hunger - could you extract an issue and what exactly causes the issue?

How common is pre-commit.sh? Given the amount of documentation that is already present, it might be better to live in a separate document that elaborates further on usage rather than having everything in the README.md

@hunger
Copy link
Contributor Author

hunger commented Oct 11, 2021

No idea how common pre-commit actually is. I personally use it a lot since it makes it easy to have very similar git hooks across different machines. Pre-commit grabs and installs everything it needs and supports a wide range of tasks for a wide range of languages:-)

Where should I put the documentation? Into a newly created docs dir?

I would remove the git hook section from the readme and put a link to the separate document into place if that is OK.

The issue is that pre-commit fails to build due to detecting dead code. I have not checked where that deny comes from though, I just removed that dead code in my local checkout.

@drahnr
Copy link
Owner

drahnr commented Oct 11, 2021

yeah, adding a docs dir and adding links from the README.md sounds good. Thank you!

Could you add an issue regarding the dead code preventing it to be used with pre-commit?

@hunger
Copy link
Contributor Author

hunger commented Oct 12, 2021

The build failure is now reported as #206

@hunger
Copy link
Contributor Author

hunger commented Oct 12, 2021

yeah, adding a docs dir and adding links from the README.md sounds good. Thank you!

I moved the CI/CD and git hook section into one "automation.md" file.

You can view them in my fork:
README.md
automation.md

@drahnr
Copy link
Owner

drahnr commented Oct 12, 2021

Merged #206 , could you rebase your changes and force push to this PR? Thank you!

Enable [pre-commit](https://pre-commit.com/) to use cargo-spellcheck
@hunger
Copy link
Contributor Author

hunger commented Oct 12, 2021

Sure, here you go.

Copy link
Owner

@drahnr drahnr left a comment

Choose a reason for hiding this comment

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

Lgtm! Thank you!

@drahnr
Copy link
Owner

drahnr commented Oct 13, 2021

REMEDY.md should probably be also moved into ./docs/ and be referenced and skimmed for duplicate information.

Other than that, LGTM!

@hunger
Copy link
Contributor Author

hunger commented Oct 13, 2021

I can go over the docs and move some things into the docs folder, but that should probably go into a different PR.

@drahnr
Copy link
Owner

drahnr commented Oct 14, 2021

Yeah, that sounds good.

I merged your changes as part of #211 and did a reflow to adhere to the width checks via:

cargo spellcheck reflow README.md

Thanks again for your PRs, looking forward to more of them 鈽猴笍

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.

None yet

2 participants