Changed logging output to always include a prefix of '[rusty-hook] '

[Mastermindaxe]

Changes

• Changed logging output to always include a prefix of '[rusty-hook] '

Related Issues

• Closes #102

[Mastermindaxe]

@calebcartwright Have implemented it as suggested in the original issue message that you posted. It kinda feels icky in the tests to have the prefix of '[rusty-hook] ' as an error message. And it is probably not right as I just noticed. I'm still unfamiliar with the syntax of closures as used in the code base. Are you invoking functions with them?

I'm gonna look into this more and then I'll hopefully have these questions answered myself :D

[Mastermindaxe]

@calebcartwright Sorry for the repeated ping but I'd like to help a bit more with this repository. I'm still far off from understanding everything, but I'm willing to learn and to put work into it. Is there anything I can help with that isn't documented as issues?

I didn't know how to contact you other than dropping a comment somewhere. Cheers!

[calebcartwright]

Thanks @Mastermindaxe! We really only need to add the prefix to the places in the code that are writing to a user-facing buffer. IIRC, there's a few print/eprint invocations in the cli code in main.rs and the build script in build.rs, as well as anywhere within the source/lib that's invoking the log function. We don't need to append anything on the error message strings within the app itself though

[Mastermindaxe]

Okay! I'll fix that next time I'm working on this.

[calebcartwright]

I'm still unfamiliar with the syntax of closures as used in the code base. Are you invoking functions with them?

More or less. There's a handful of i/o related capabilities that rusty-hook needs (writing hook & config files, reading config files, etc.), and since rusty-hook itself can and does perform it's respective actions in a stateless, immutable way, I opted to implement in a more functional-like flavor. Functions within rusty-hook that need to do one of those i/o ops accept a parameter that's a closure which the function call to do that i/o thing.

For example, the config function that's used to check for a rusty-hook config file needs to actually see if any of the supported options are on the file system, a closure named file_exists in this case, which the function leverages to make that determination.

https://github.com/swellaby/rusty-hook/blob/99673e9816ff687699a29bbe9dcdfc480f331000/src/config.rs#L17-L36

This helps both with encapsulation/reuse, but perhaps most importantly it makes things really easy to unit test in isolation without the actual i/o hits. There are some better crates available today that have decent support for mocking things out, but they require extra setup and add verbosity. Another common approach for IoC and testability would be to define traits and structs with implementations of said traits which also works great, but I just felt doing so would be a little heavy and involved for something as simple as rusty-hook

[calebcartwright]

Sorry for the repeated ping but I'd like to help a bit more with this repository. I'm still far off from understanding everything, but I'm willing to learn and to put work into it.

Excellent! Much appreciated

Is there anything I can help with that isn't documented as issues? There's quite a few existing issues that could use assistance if you're interested (those with help wanted and/or good first issue labels are prime candidates, though don't feel restricted to just those). I can't really think of anything notable offhand that isn't already captured as an issue, perhaps outside the need for a functional testing suite but that's too abstract at this point.

I didn't know how to contact you other than dropping a comment somewhere. Cheers!

No worries, feel free to ping on issues and @ mention. We don't have an open chat server anywhere so for now GH is the best place!

[codecov[bot]]

Codecov Report

Merging #125 into master will not change coverage. The diff coverage is n/a.

@@            Coverage Diff            @@
##            master      #125   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            4         4           
  Lines          143       143           
=========================================
  Hits           143       143           

Impacted Files	Coverage Δ
src/rusty_hook.rs	100.00% <ø> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 5aa74d5...b3f4295. Read the comment docs.

[Mastermindaxe]

Output looks like this for example:

rusty-hook on  improve-logging-output is 📦 v0.11.2 via 🦀 v1.46.0 
❯ cargo run -- run --hook pre-push
    Finished dev [unoptimized + debuginfo] target(s) in 0.01s
     Running `target/debug/rusty-hook run --hook pre-push`
[rusty-hook] Found configured hook: pre-push
[rusty-hook] Running command: cargo fmt -- --check

[Mastermindaxe]

Not sure how to test if I didn't forget a "[rusty-hook] " somewhere tbh as it gets directly printed to the console and only those parts are affected. How would I go about testing that? Or do we just ignore it? @calebcartwright

As soon as this PR is okay with you I'm gonna do a commit squash!

[calebcartwright]

The CI check failures were spurious, seems to be some weird networking issues with Azure DevOps today. Have restarted them, and will take a look at the updates later tonight

[calebcartwright]

Not sure how to test if I didn't forget a "[rusty-hook] " somewhere tbh as it gets directly printed to the console and only those parts are affected. How would I go about testing that? Or do we just ignore it?

The shell scripts are covered as updated here, and not worried about unit tests for the main.rs bin and build script. We could add/enhance the unit tests for hook execution though within the log closure to validate the message (btw that first param should definitely be something like message or msg, I must've done a bad copy/paste job there before).

I'm not concerned about making that change as part of this PR, though if you feel like it feel free to make the change here or submit a follow up PR.

https://github.com/swellaby/rusty-hook/blob/99673e9816ff687699a29bbe9dcdfc480f331000/src/rusty_hook_test.rs#L191-L216

[Mastermindaxe]

This should be good to merge now. (If I rebased it correctly 👀) @calebcartwright

[calebcartwright]

Thanks!