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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Recommend setting up check_line_lengths as a pre-commit script #889

Merged
merged 3 commits into from
Oct 1, 2020

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Sep 24, 2020

I've run into lots of annoying failures from this.

  • Make it runnable without arguments
  • Add it in the README

Open questions: How does this work on Windows (if at all)?

@LeSeulArtichaut
Copy link
Contributor

Maybe we could also suggest setting up the editor to do the formatting?

@jyn514
Copy link
Member Author

jyn514 commented Sep 24, 2020

Maybe we could also suggest setting up the editor to do the formatting?

I'm not sure how to do that - in vim I use gqq but I don't know if VSCode or other editors have an equivalent.

@jyn514
Copy link
Member Author

jyn514 commented Sep 29, 2020

ping @spastorino - can we get this merged? I find it really useful and I'm sure others would too.

@jyn514 jyn514 added the waiting-on-review This PR is waiting for a reviewer to verify its content label Sep 29, 2020
@LeSeulArtichaut
Copy link
Contributor

@jyn514 did you solve the "does this work on Windows" question?

@jyn514
Copy link
Member Author

jyn514 commented Sep 29, 2020

It looks like the fix is cd .git/hooks && New-Item -Path ../../.git_hooks/* -ItemType SymbolicLink -Value . && cd ../.., I'll add that to the docs.

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
jyn514 and others added 3 commits September 30, 2020 21:04
I've run into lots of annoying failures from this.

- Make it runnable without arguments
- Add it in the README
Co-authored-by: Yuki Okushi <huyuumi.dev@gmail.com>
@jyn514
Copy link
Member Author

jyn514 commented Oct 1, 2020

Updated!

@JohnTitor
Copy link
Member

Looks great now, thanks!

@jyn514 jyn514 merged commit c58ff8d into rust-lang:master Oct 1, 2020
@jyn514 jyn514 deleted the pre-commit branch October 1, 2020 05:28
@spastorino
Copy link
Member

@jyn514 👍, does the script check files when the ignore directive is used?.

@jyn514
Copy link
Member Author

jyn514 commented Oct 7, 2020

It works the same way as it did before. I don't see any logic for ignoring lines, that seems like a useful follow-up though.

@spastorino
Copy link
Member

Just in case, I was referring to // ignore-tidy-linelength which we use mainly in tests like in https://github.com/rust-lang/rust/blob/master/src/test/ui/stability-attribute/generics-default-stability.rs#L1.
I was guessing (didn't read the code) that this pre-commit hook would check that the to be committed files do not have more than 100 length lines but for this to be correct we would need to ignore the ones that use that directive.

@jyn514
Copy link
Member Author

jyn514 commented Oct 7, 2020

@spastorino hmm, I'm not sure this makes sense to add. // ignore-tidy-linelength is not a comment in markdown, it will show up in the user-facing docs, and I don't think any of the existing markdown files use it.

@spastorino
Copy link
Member

I should've read what this was about 😂, I thought it was a script to check Rust source files length before commit, that's why I was thinking about ignoring those. Anyway, nevermind.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-on-review This PR is waiting for a reviewer to verify its content
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants