-
Notifications
You must be signed in to change notification settings - Fork 11
Added gix support! #162
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
base: main
Are you sure you want to change the base?
Added gix support! #162
Conversation
cococonscious
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! It looks good overall, there's just some minor things I'd like to be different.
| pub autocomplete: bool, | ||
| pub breaking_changes: bool, | ||
| pub commit_types: IndexMap<String, CommitType>, | ||
| pub commit_scopes: IndexMap<String, CommitScope>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please isolate the changes to the configuration from the gix support, as #161 is already introducing that feature.
This also applies to the other references to configurable commit scopes of course.
| temp_dir: &std::path::Path, | ||
| message: &'static str, | ||
| ) -> Result<(), Box<dyn Error>> { | ||
| Command::new("git") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a huge fan of running system commands if there is a way to do it programmatically, can you try to implement it with gix?
|
|
||
| [features] | ||
| vendored-openssl = ["git2/vendored-openssl"] | ||
| vendored-openssl = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose this feature can be removed as it was only needed for git2
| assert_cmd = "2.0.16" | ||
| predicates = "3.1.2" | ||
| tempfile = "3.12.0" | ||
| rusty-hook = "^0.11.2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I replaced rusty-hook with prek recently as the former seems abandoned, please remove the dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Odd, it was forcing me to install it to commit... is prek a drop-in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I don't think so, prek is a drop-in for pre-commit
| for info in walk.all()? { | ||
| let info = info?; | ||
|
|
||
| // Get the commit object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These comments seem quite redundant since the code is very readable in my opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha they totally are, just included them to help me work through the conversion
Codecov Report✅ All modified and coverable lines are covered by tests.
🚀 New features to boost your workflow:
|
No description provided.