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

Allow multiple commands issue 112 #116

Conversation

Mastermindaxe
Copy link
Contributor

Changes

  • Allow multiple commands for a given Git hook
  • Write tests

Related Issues

@Mastermindaxe
Copy link
Contributor Author

@calebcartwright My first try on this :D It seems to be working fine. There's one thing that's kinda bothering me though and not very ergonomic.

src/config.rs Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Sep 22, 2020

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #116   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            4         4           
  Lines          129       141   +12     
=========================================
+ Hits           129       141   +12     
Impacted Files Coverage Δ
src/config.rs 100.00% <100.00%> (ø)
src/hooks.rs 100.00% <0.00%> (ø)
src/rusty_hook.rs 100.00% <0.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 918bc57...3b17e95. Read the comment docs.

@Mastermindaxe
Copy link
Contributor Author

Mastermindaxe commented Sep 22, 2020

And is there any way for me to see what is not covered by the tests? To fix that code coverage issue. I haven't worked with code coverage tools in Rust yet 😅 This PR is probably more work for you than if you just would have done it yourself 🙈

EDIT: I guess #18 would be helpful. I'm planning to do some more OSS work so I'd be glad to help out. (If I can with my limited knowledge)

@calebcartwright
Copy link
Member

And is there any way for me to see what is not covered by the tests? To fix that code coverage issue. I haven't worked with code coverage tools in Rust yet sweat_smile

I think that the Codecov links should be public (https://codecov.io/gh/swellaby/rusty-hook/pull/116/diff?src=pr&el=tree#diff-c3JjL2NvbmZpZy5ycw==). The Coverage is being generated from tarpaulin which is pretty solid, though occasionally imperfect and only available on Linux. If you'd like to run locally and on Mac then I believe the Docker option can work, and on Windows using WSL should work too.

This PR is probably more work for you than if you just would have done it yourself see_no_evil

Not at all! I'd much rather have others contribute and be able to just review than have to do it myself 😄

@calebcartwright
Copy link
Member

EDIT: I guess #18 would be helpful.

Lol exactly!

I'm planning to do some more OSS work so I'd be glad to help out. (If I can with my limited knowledge)

That's great to hear! It's good timing as well with Hacktoberfest fast approaching. There's plenty of work to do here if you're interested and have time

src/config_test.rs Outdated Show resolved Hide resolved
@Mastermindaxe
Copy link
Contributor Author

Mastermindaxe commented Sep 23, 2020

That's great to hear! It's good timing as well with Hacktoberfest fast approaching. There's plenty of work to do here if you're interested and have time

Hell yeah! I have the feeling that this is a small enough repo to get me started to OSS. This is my first PR with code in it ever and I'm very happy how easy it was to work with this repo

@Mastermindaxe
Copy link
Contributor Author

Not at all! I'd much rather have others contribute and be able to just review than have to do it myself 😄

Glad to hear that!

@calebcartwright
Copy link
Member

calebcartwright commented Sep 24, 2020

Thanks @Mastermindaxe! This is good to go, though would you mind squashing up the commits a bit? Would prefer avoiding adding1db457b2f8c0ce3ab8d45417d308dacd4f667338 and 2d67771 to the history. Alternatively I can squash while merging

@Mastermindaxe
Copy link
Contributor Author

Mastermindaxe commented Sep 24, 2020 via email

@calebcartwright
Copy link
Member

I've never squashed commits so feel free to do that while merging sweat_smile I'm gonna look into it though so I can do it next time! Thanks!

No worries, I'll take care of it later today. You may find these helpful for future reference:

https://thoughtbot.com/blog/git-interactive-rebase-squash-amend-rewriting-history
http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html

Copy link
Member

@calebcartwright calebcartwright left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@calebcartwright calebcartwright merged commit 255b774 into swellaby:master Sep 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow multiple commands for a given Git hook
2 participants