Using a custom script

[JonathanWoollett-Light]

Using a custom script

[calebcartwright]

No worries, thanks for reaching out!

[calebcartwright]

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:

• You're not the first person that's wanted to use unstable features on stable, and you certainly won't be the last 😄 While I'm personally somewhat sympathetic to this for rustfmt config options, using unstable things on stable runs starkly counter to Rust's core ethos and that's a big part of why the many past requests/arguments have been rejected
• People commonly have negative and incorrect connotations around nightly toolchains. There's certainly contexts where it's understandable that a project may want to require a stable toolchain, but the resultant binary output from a nightly compiler is just as trustworthy with the same guarantees as if it had been built with a stable compiler. The bigger considerations tend to be whether the nightly compiler may fail to compile code, or if/how much rusk one is willing to take on by writing code that is predicated on gated, unstable features (e.g. GATs) which could potentially be removed.

[calebcartwright]

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?

[calebcartwright]

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

[JonathanWoollett-Light]

Seems like I was just missing the sh, sorry about that. Also thanks for rustfmt advice.

[JonathanWoollett-Light]

@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"

[calebcartwright]

@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.

[JonathanWoollett-Light]

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).

[calebcartwright]

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

[JonathanWoollett-Light]

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.