Refactor rules_check.py git operations #98
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Pull request overview
This PR refactors approximately 200 lines of git comparison logic from rules_check.py into a new reusable module git_utils.py. The refactoring introduces an object-oriented design using abstract base classes and factory pattern to encapsulate different git comparison strategies.
Changes:
- Created
GitComparatorabstract base class with three concrete implementations (CompareToBase, CompareToDefaultTip, CompareToPrompt) - Added factory function
get_comparator()for selecting the appropriate comparator - Updated
rules_check.pyto use the new comparator interface, removing duplicate git operation code - Enhanced module documentation with detailed call sites and related files information
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/deepwork/core/git_utils.py | New module containing GitComparator abstraction and implementations for three comparison strategies (base, default_tip, prompt) with helper functions for git operations |
| src/deepwork/hooks/rules_check.py | Removed ~350 lines of git operation functions, replaced with calls to get_comparator(); updated header documentation with CALL SITES and RELATED FILES sections |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…s.py - Create GitComparator abstract base class with common interface - Add RefBasedComparator intermediate class to eliminate duplication between CompareToBase and CompareToDefaultTip (they now only define _get_ref()) - Extract helper functions: _parse_file_list(), _run_git(), _collect_all_files() - CompareToPrompt remains separate due to its file-based baseline approach - Update rules_check.py to use the new comparator interface - Add detailed header comments documenting call sites and related files The refactored code reduces ~370 lines of repetitive git operations to ~275 lines with proper abstraction and DRY principles applied.
nhorton
force-pushed
the
claude/refactor-git-operations-2WxqO
branch
from
5da6c0d to
8921236
Compare
- Fix CompareToPrompt.get_created_files() to include files added in commits since the baseline ref (was only checking staged/untracked files) - Simplify git operations by using a single `git diff --cached ref` command after staging all changes, instead of multiple separate commands - Add _get_all_changes_vs_ref() helper that captures committed, staged, and untracked changes in one command - Add comprehensive test suite for git_utils module (35 tests)
The new code was incorrectly using .last_head_ref for created files detection when available. This caused uncommitted files from before the prompt to be incorrectly flagged as "created". The fix restores the original behavior: always use .last_work_tree for created files detection, since it contains the actual list of files that existed at prompt time (including uncommitted files). Added a test that explicitly verifies this behavior.
Reorganize test_git_utils.py to clearly separate requirements tests from implementation tests, following the pattern in test_install_requirements.py: Requirements tested: - REQ-001: All comparators MUST implement GitComparator interface - REQ-002: get_comparator() MUST return correct comparator for each mode - REQ-003: CompareToPrompt.get_created_files() MUST use .last_work_tree - REQ-004: Created files are those NOT present in baseline The requirements tests are placed at the top with DO NOT MODIFY warnings, while implementation tests are grouped separately below.
- Fix misleading comments about "current files" to clarify they are files that differ from HEAD - Add defensive programming comments for _get_untracked_files() calls - Remove incomplete test_get_changed_files_in_real_repo test - Fix weak assertion in test_strips_outer_whitespace_only
….com/Unsupervisedcom/deepwork into claude/refactor-git-operations-2WxqO
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.