swellaby / rusty-hook

git hook manager, geared toward Rust projects
MIT License
206 stars 11 forks source link

Inline git arguments #122

Closed Absolucy closed 3 years ago

Absolucy commented 3 years ago

Changes

Related Issues

codecov[bot] commented 3 years ago

Codecov Report

Merging #122 into master will not change coverage. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #122   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            4         4           
  Lines          137       139    +2     
=========================================
+ Hits           137       139    +2     
Impacted Files Coverage Δ
src/rusty_hook.rs 100.00% <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 755cd44...18c8294. Read the comment docs.

calebcartwright commented 3 years ago

Could you update the tests to reflect the param change on run to fix the CI checks?

Absolucy commented 3 years ago

Done

calebcartwright commented 3 years ago

Done

Great! Any thoughts on my last comment https://github.com/swellaby/rusty-hook/pull/122#discussion_r496281140?

Absolucy commented 3 years ago

I removed the warning.

calebcartwright commented 3 years ago

Thanks @aspenluxxxy! The last outstanding item is the proposed variable/token sequence for substitution.

It's currently %@, but I still think we need to change this to something else. I really just want to avoid a scenario where rusty-hook is doing a replacement that's unintended and undesired by the user. I had a quick offline chat with another maintainer to see if we could think of any real scenarios where a user could potentially have %@ within a hook and not want it replaced with the git args (lemme know if I forgot anything or summarized inaccurately @beverts312).

We did come up with a few potential scenarios, and although they're fairly edge case-y and not terribly likely, the fact that we were able to come up with them pretty quickly makes me think there could indeed be some "real" use cases.

As such I'd like to see the replacement string changed from %@ to something else. I'm open to anything really, but think that something longer will be better and less likely to collide with any real uses cases. For example %@rusty, %@rusty_hook, %@rhga, etc.

Absolucy commented 3 years ago

The %@ was originally from Objective-C, as it was simple, short, and unlikely to collide.

How about %rh!?

calebcartwright commented 3 years ago

I like that better, thanks :+1:

Absolucy commented 3 years ago

I keep pushing to the wrong branch, agh