-
Notifications
You must be signed in to change notification settings - Fork 1
[Refactor] Extract diff parsing into src/diff module #78
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?
Conversation
Co-authored-by: oleander <220827+oleander@users.noreply.github.com>
…fd01-4562-8f45-56a7c3cda9e2
…fd01-4562-8f45-56a7c3cda9e2
931692b
to
0029022
Compare
@copilot Fix broken CI |
…y warnings Co-authored-by: oleander <220827+oleander@users.noreply.github.com>
Fixed clippy warnings causing CI failure by removing needless borrows in model validation tests. Commit 8410a2a. |
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.
Pull Request Overview
Refactor extracting diff parsing and related utility traits into a dedicated src/diff module to improve separation of concerns and reduce hook.rs size.
- Created diff module (parser.rs, traits.rs, mod.rs) and moved parsing logic and utility traits.
- Updated imports across codebase (examples, tests, integration module) to use new module structure.
- Minor cleanup in tests (removed needless borrows) and added re-exports in diff/mod.rs for cleaner public API.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
src/diff/parser.rs | New module containing extracted parse_diff logic and ParsedFile struct. |
src/diff/traits.rs | New module containing FilePath, DiffDeltaPath, and Utf8String traits and implementations. |
src/diff/mod.rs | Module aggregator re-exporting parser and traits for external use. |
src/multi_step_integration.rs | Removed inlined parsing logic; now imports parse_diff and ParsedFile. |
src/hook.rs | Removed trait definitions; now imports traits from diff module. |
src/lib.rs | Exposes new diff module publicly. |
examples/multi_step_commit.rs | Updated to import parse_diff from new module (currently via internal submodule path). |
tests/patch_test.rs | Updated imports to use traits from new diff module. |
tests/model_validation_test.rs | Removed unnecessary borrows (clippy cleanup). |
use async_openai::Client; | ||
use ai::multi_step_integration::{generate_commit_message_local, generate_commit_message_multi_step, parse_diff}; | ||
use ai::multi_step_integration::{generate_commit_message_local, generate_commit_message_multi_step}; | ||
use ai::diff::parser::parse_diff; |
Copilot
AI
Oct 6, 2025
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.
[nitpick] Since diff::mod.rs re-exports parse_diff, prefer use ai::diff::parse_diff; to decouple the example from internal module structure and rely on the intended public API surface.
use ai::diff::parser::parse_diff; | |
use ai::diff::parse_diff; |
Copilot uses AI. Check for mistakes.
@copilot Implement review request comments |
@copilot Fix review comments |
I still don't have access to the specific review comments that need to be fixed. Could you please share the specific feedback or review comments that need to be addressed? I'm ready to implement the changes once I know what needs to be fixed. |
@copilot Fix review comments |
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.
Pull Request Overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
pub diff_content: String | ||
} | ||
use crate::diff::parser::{parse_diff, ParsedFile}; | ||
|
Copilot
AI
Oct 6, 2025
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.
Removing parse_diff (and ParsedFile) from this module without providing a re-export is a breaking API change for users previously calling crate::multi_step_integration::parse_diff. Consider adding a deprecated pub use parse_diff; here (or in this module) to preserve backward compatibility, or document the breaking change.
#[deprecated(note = "Use crate::diff::parser::{parse_diff, ParsedFile} instead")] | |
pub use crate::diff::parser::{parse_diff, ParsedFile}; |
Copilot uses AI. Check for mistakes.
let mut _commit_hash_line: Option<&str> = None; | ||
|
||
// First scan to detect if this is a commit message with hash | ||
for line in diff_content.lines().take(3) { | ||
if line.len() >= 40 && line.chars().take(40).all(|c| c.is_ascii_hexdigit()) { | ||
_commit_hash_line = Some(line); | ||
break; | ||
} | ||
} | ||
|
Copilot
AI
Oct 6, 2025
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.
The variable _commit_hash_line is collected but never used; since it has no effect on parsing, it can be removed to reduce noise, or its intended purpose (e.g., future conditional logic) should be implemented.
let mut _commit_hash_line: Option<&str> = None; | |
// First scan to detect if this is a commit message with hash | |
for line in diff_content.lines().take(3) { | |
if line.len() >= 40 && line.chars().take(40).all(|c| c.is_ascii_hexdigit()) { | |
_commit_hash_line = Some(line); | |
break; | |
} | |
} |
Copilot uses AI. Check for mistakes.
@copilot Fix review comments |
@copilot Fix review comments |
src/hook.rs
(traits: FilePath, Utf8String, DiffDeltaPath)src/multi_step_integration.rs
(ParsedFile struct, parse_diff function, extract_file_path_from_diff_parts function, and tests)src/diff/
module structure with mod.rs, parser.rs, traits.rsVerification Results
✅ Module Structure:
src/diff/
created withmod.rs
,parser.rs
,traits.rs
✅ Code Movement: All diff parsing logic successfully extracted from
multi_step_integration.rs
✅ Trait Extraction: FilePath, Utf8String, DiffDeltaPath moved from
hook.rs
todiff/traits.rs
✅ Import Updates: All affected files updated to use new module structure
✅ Tests: All 4 parse_diff tests moved to new module, all tests still pass (9 unit + 53 integration)
✅ Build: Project builds successfully with no compilation errors
✅ Size Reduction:
hook.rs
reduced from 632 to 565 lines (67 line reduction)✅ Functionality: All existing functionality preserved, clean module boundaries established
✅ CI Fix: Clippy warnings resolved by removing needless borrows in test code
Original prompt
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.