Merged
Conversation
Reviewer's GuideAdjusts check behavior so Ruff config drift takes precedence over pre-commit hook drift, adds a regression test for this exit-code behavior, updates documentation diagrams and styling to match the new logic and Tailwind-inspired palette, and bumps project metadata for the 0.1.2 release. Sequence diagram for ruff sync check with pre-commit precedencesequenceDiagram
actor User
participant CLI as ruff_sync_check_CLI
participant Core as core_check
participant Upstream as upstream_merge
participant PreCommit as sync_pre_commit
User->>CLI: invoke_check_command(args)
CLI->>Core: check(args)
Core->>Upstream: merge_multiple_upstreams(source_doc)
Upstream-->>Core: merged_doc
Core->>Core: compare_semantic_or_text(source_doc, merged_doc)
alt Ruff_config_out_of_sync
Core-->>CLI: return_exit_code(1)
CLI-->>User: Exit 1 (Out of Sync)
else Ruff_config_in_sync
alt pre_commit_flag_not_set
Core-->>CLI: return_exit_code(0)
CLI-->>User: Exit 0 (In Sync)
else pre_commit_flag_set
Core->>PreCommit: sync_pre_commit(cwd, dry_run=True)
alt hook_version_in_sync
Core-->>CLI: return_exit_code(0)
CLI-->>User: Exit 0 (In Sync)
else hook_version_out_of_sync
Core-->>CLI: print_warning_and_return(2)
CLI-->>User: Exit 2 (Hook Drift)
end
end
end
Flow diagram for ruff sync check exit-code precedenceflowchart TD
Start([Start check]) --> Compare{Ruff config in sync?}
Compare -- No --> Exit1([Exit 1: Ruff config out of sync])
Compare -- Yes --> PCFlag{--pre-commit flag set?}
PCFlag -- No --> Exit0a([Exit 0: In sync])
PCFlag -- Yes --> HookMatch{Hook version in sync?}
HookMatch -- Yes --> Exit0b([Exit 0: In sync])
HookMatch -- No --> Exit2([Exit 2: Hook drift])
%% Styling
style Start fill:#2563eb,color:#fff,stroke:#1d4ed8
style Compare fill:#ca8a04,color:#fff,stroke:#a16207
style PCFlag fill:#ca8a04,color:#fff,stroke:#a16207
style HookMatch fill:#ca8a04,color:#fff,stroke:#a16207
style Exit0a fill:#16a34a,color:#fff,stroke:#15803d
style Exit0b fill:#16a34a,color:#fff,stroke:#15803d
style Exit1 fill:#dc2626,color:#fff,stroke:#b91c1c
style Exit2 fill:#ea580c,color:#fff,stroke:#c2410c
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The
checkfunction now duplicates the pre-commit drift check and message in both the semantic and string-equality branches; consider extracting this into a small helper to keep the control flow and messaging consistent and easier to maintain. - The flowchart in
docs/usage.mdstill describes the pre-commit branch, while the simplified flowchart inREADME.mdnow omits it; it may be worth aligning these diagrams so the high-level behavior around--pre-commitis described consistently across docs.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `check` function now duplicates the pre-commit drift check and message in both the semantic and string-equality branches; consider extracting this into a small helper to keep the control flow and messaging consistent and easier to maintain.
- The flowchart in `docs/usage.md` still describes the pre-commit branch, while the simplified flowchart in `README.md` now omits it; it may be worth aligning these diagrams so the high-level behavior around `--pre-commit` is described consistently across docs.
## Individual Comments
### Comment 1
<location path="src/ruff_sync/core.py" line_range="913" />
<code_context>
if source_val == merged_val:
print("✅ Ruff configuration is semantically in sync.")
- return 2 if out_of_sync else 0
+ if getattr(args, "pre_commit", False) and not sync_pre_commit(
+ pathlib.Path.cwd(), dry_run=True
+ ):
+ print("⚠️ Pre-commit hook version is out of sync!")
+ return 2
+ return 0
elif source_doc.as_string() == merged_doc.as_string():
print("✅ Ruff configuration is in sync.")
- return 2 if out_of_sync else 0
+ if getattr(args, "pre_commit", False) and not sync_pre_commit(
+ pathlib.Path.cwd(), dry_run=True
+ ):
</code_context>
<issue_to_address>
**suggestion:** Avoid duplicating the pre-commit sync check logic in both success branches.
The `getattr(args, "pre_commit", False) and not sync_pre_commit(..., dry_run=True)` block is duplicated in both branches, which risks them diverging and requires double edits for any future behavior changes. Extract this into a shared helper or a single block at the end of the function (e.g., a small function or a final `if getattr(args, "pre_commit", False): ...` before returning) so the pre-commit behavior stays consistent and non-duplicated.
Suggested implementation:
```python
def _check_pre_commit_sync(args) -> int | None:
"""Return exit code 2 if pre-commit hook version is out of sync, otherwise None.
Shared helper to avoid duplicating the pre-commit synchronization logic.
"""
if getattr(args, "pre_commit", False) and not sync_pre_commit(
pathlib.Path.cwd(), dry_run=True
):
print("⚠️ Pre-commit hook version is out of sync!")
return 2
return None
async with httpx.AsyncClient() as client:
merged_doc = await _merge_multiple_upstreams(
merged_doc,
```
```python
if source_val == merged_val:
print("✅ Ruff configuration is semantically in sync.")
exit_code = _check_pre_commit_sync(args)
if exit_code is not None:
return exit_code
return 0
```
```python
elif source_doc.as_string() == merged_doc.as_string():
print("✅ Ruff configuration is in sync.")
exit_code = _check_pre_commit_sync(args)
if exit_code is not None:
return exit_code
```
</issue_to_address>
### Comment 2
<location path="tests/test_check.py" line_range="369-370" />
<code_context>
+ assert "is out of sync!" in captured.out
+ assert '-target-version = "py310"' in captured.out
+ assert '+target-version = "py311"' in captured.out
+ # Pre-commit drift should NOT be reported if config drift was found and resulted in exit 1
+ assert "pre-commit Ruff hook is out of sync" not in captured.out
+
+
</code_context>
<issue_to_address>
**issue (testing):** The assertion is checking for the absence of an outdated message string and might not protect against regressions in the new behavior.
This test is still keyed to the old warning text (`"pre-commit Ruff hook is out of sync"`), while the implementation now emits `"⚠️ Pre-commit hook version is out of sync!"`. As a result, it won’t fail if the new warning appears. To properly guard against regressions, assert that the current warning string is not present in `captured.out` instead.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #119 +/- ##
==========================================
- Coverage 90.58% 90.52% -0.06%
==========================================
Files 4 4
Lines 722 728 +6
==========================================
+ Hits 654 659 +5
- Misses 68 69 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
|
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary by Sourcery
Prepare the 0.1.2 release by refining pre-commit drift handling, updating documentation diagrams, and bumping project metadata.
Bug Fixes:
ruff-sync check.Enhancements:
checkcommand logic to only perform and report pre-commit hook drift checks when the Ruff configuration is already in sync.Build:
Documentation:
Tests: