Skip to content

Conversation

@philocalyst
Copy link

No description provided.

Copy link
Owner

@cococonscious cococonscious left a 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>,
Copy link
Owner

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")
Copy link
Owner

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 = []
Copy link
Owner

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"
Copy link
Owner

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.

Copy link
Author

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?

Copy link
Owner

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
Copy link
Owner

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.

Copy link
Author

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
Copy link

codecov bot commented Dec 10, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.

Files with missing lines Coverage Δ
src/lib/commit.rs 100.00% <ø> (ø)
src/lib/config.rs 100.00% <100.00%> (ø)
src/lib/questions.rs 98.06% <100.00%> (+0.88%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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.

2 participants