Closed JonathanWoollett-Light closed 2 years ago
Your script is working fine for me on linux using pre-commit = sh testing-this.sh
, including output and exit code (i added a non-zero exit to the script to confirm).
Would you mind elaborating with more details?
Also, somewhat tangential, but with my rustfmt team lead hat on, I wanted to note that you can't cargo fmt
an individual file, and you'd want to run rustfmt ...
directly
Seems like I was just missing the sh
, sorry about that. Also thanks for rustfmt
advice.
No worries, thanks for reaching out!
@calebcartwright I was not sure if worth re-opening or a new issue, but I am experiencing perhaps a new error. When making commit on https://github.com/JonathanWoollett-Light/testing-git-hooks which results in pre-commit.sh
error-ing the commits still continue through:
PS C:\Users\jcawl\Projects\testing-git-hooks> git add *
PS C:\Users\jcawl\Projects\testing-git-hooks> git commit -m "test"
Found configured hook: pre-commit
Running command: sh pre-commit.sh
Fetching advisory database from `https://github.com/RustSec/advisory-db.git`
Loaded 417 security advisories (from C:\Users\jcawl\.cargo\advisory-db)
Updating crates.io index
Scanning Cargo.lock for vulnerabilities (13 crate dependencies)
error: expected expression, found keyword `as`
--> \\?\C:\Users\jcawl\Projects\testing-git-hooks\src\main.rs:7:21
|
7 | let _i = 12u32; as as dasd
| ^^ expected expression
warning: unused manifest key: logging
Checking testing-git-hooks v0.1.0 (C:\Users\jcawl\Projects\testing-git-hooks)
error: expected expression, found keyword `as`
--> src\main.rs:7:21
|
7 | let _i = 12u32; as as dasd
| ^^ expected expression
error: could not compile `testing-git-hooks` due to previous error
[master 0093fbe] test
1 file changed, 1 insertion(+), 1 deletion(-)
PS C:\Users\jcawl\Projects\testing-git-hooks>
The respective pre-commit.sh
:
#!/bin/bash
# I don't think there is a reasonable way to do these more specifically
cargo audit
# For every staged file
for i in $(git diff --name-only --cached); do
# Get the extension
filename=$(basename -- "$i")
extension="${filename##*.}"
if [ "$extension" = "rs" ]; then
# Maybe also consider https://crates.io/crates/cargo-spellcheck
# TODO We need to have some discussion on what configs to use for `fmt` and `clippy`.
# TODO Put the fmt config arguments in different lines.
# Run `cargo fmt` for this file
rustfmt $i --config comment_width=100,wrap_comments=true,format_code_in_doc_comments=true,format_strings=true,license_template_path=./license.txt,imports_granularity=Module,normalize_comments=true,normalize_doc_attributes=true,group_imports=StdExternalCrate
# Run `cargo-clippy` for this file
cargo-clippy $i -- -D warnings \
-D clippy::as_conversions \
-D clippy::map_err_ignore \
-D clippy::large_types_passed_by_value \
-D clippy::missing_docs_in_private_items \
-D clippy::used_underscore_binding \
-D clippy::wildcard_dependencies \
-D clippy::wildcard_imports
fi
if [ "$extension" == "py" ]; then
# Run `black` for this file
black $i
fi
# Add changes to this file (as a result of formatting) to the commit.
git add $i
done
.rust-hook.toml
:
[hooks]
pre-commit = "sh pre-commit.sh"
Cargo.toml
:
[package]
name = "testing-git-hooks"
version = "0.1.0"
edition = "2021"
# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
[dependencies]
[dev-dependencies]
rusty-hook = "0.11.2"
@JonathanWoollett-Light haven't had a chance to look closely, but I suspect you will probably want to consider incorporating some set
options at the top of your script (perhaps set -e
and/or set -o pipefail
)
# TODO Put the fmt config arguments in different lines.
Would actually suggest you just plop a rustfmt config file in your repo and remove the CLI settings. Otherwise if you do both you'll inevitably end up with drift/duplicative management, or if you only set them here you may create some less-than-ideal inner dev loops due to e.g. editor-format-on-save behaviors formatting with a different rule set.
set -e
fixed the issue thank you.
Would actually suggest you just plop a rustfmt config file in your repo and remove the CLI settings. Otherwise if you do both you'll inevitably end up with drift/duplicative management, or if you only set them here you may create some less-than-ideal inner dev loops due to e.g. editor-format-on-save behaviors formatting with a different rule set.
The problem here is that passing the arguments on command line for many of them doesn't require the nightly toolchain while using a config file does. It is very strange but for now since the project cannot use the nightly toolchain this is the approach (I changed it to use fmt.toml
then just load them from their with bash and pass them as command line arguments).
The problem here is that passing the arguments on command line for many of them doesn't require the nightly toolchain while using a config file does. It is very strange
Yes, and it's been argued that it's a bug that rustfmt on stable will accept nightly only config options provided from the CLI. I can't guarantee I won't have to remove that at some point down the road, so buyer beware if you want to stick with it
I would argue if the tool doesn't require the nightly toolchain and these options are simply unstable, it should not require the nightly toolchain for them to work, but rather a different option e.g. unstable_features=true
. Given almost all projects will avoid the nightly toolchain but most would want simple options like wrapping comments e.g. wrap_comments
and would not consider it a significant security issue.
Thanks for sharing your perspective.
I want to be mindful about having discussions or debate about one project/tool in the repository of a different project/tool. I happen to be a maintainer of both, so I just wanted to share some tips and food for thought.
However, I'll leave with a few thoughts on the topic:
Question I have a script which reformats, checks and updates staged files:
I can run this with
pre-commit = "testing-this.sh"
but this doesn't propagate the outputs or the exit code of my script.How could I implement this like any other command e.g.
cargo fmt
?