Uh oh!
There was an error while loading. Please reload this page.
Doc update + fix format-patch - #985
Conversation
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughRefactors patch formatting to operate on parsed PatchSet objects: adds parse/convert/dump utilities, updates add_prefix_to_patch to accept/return PatchSet, and changes format_patch to determine target patch type from subproject, convert, prefix, and serialize the patch before writing. Changes
Sequence DiagramsequenceDiagram
participant FC as format_patch command
participant P as dfetch.vcs.patch utilities
participant SP as SubProject type
FC->>P: parse_patch(file_path)
P-->>FC: PatchSet
FC->>SP: _determine_target_patch_type(subproject)
SP-->>FC: target_type (GIT / SVN / PLAIN)
FC->>P: convert_patch_to(PatchSet, target_type)
P-->>FC: converted PatchSet
FC->>P: add_prefix_to_patch(PatchSet, prefix)
P-->>FC: prefixed PatchSet
FC->>P: dump_patch(PatchSet)
P-->>FC: serialized patch text
FC->>FC: write to output file
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@dfetch/vcs/patch.py`:
- Around line 310-313: The header construction needs to special-case /dev/null
to avoid producing double slashes; update the loop that sets file.header for
items in patch.items so that when file.source == b"/dev/null" you do not prefix
it with b"a/" (and likewise when file.target == b"/dev/null" you do not prefix
it with b"b/"), building the header from chosen left/right parts (e.g., left =
file.source if file.source == b"/dev/null" else b"a/" + file.source; right =
file.target if file.target == b"/dev/null" else b"b/" + file.target) and then
set file.header = [b"diff --git " + left + b" " + right + b"\n"].
- Around line 304-321: Change the two separate if branches in convert_patch_to
to an if/elif (check patch_ng.GIT then elif patch_ng.SVN) so the GIT and SVN
handling are explicit and mutually exclusive, and after attempting conversion
validate the required_type: if it is not one of patch_ng.GIT or patch_ng.SVN
raise a clear error (e.g., ValueError) instead of silently setting patch.type;
ensure you still update each file.header and file.type inside the respective
branch and only set patch.type = required_type when a known conversion was
performed.
🧹 Nitpick comments (2)
tests/test_patch.py (1)
365-366: Minor type mismatch:parse_patchexpectsstrbut receivespathlib.Path.
original_patch_fileis apathlib.Path(fromtmp_path / "original.patch"), butparse_patch(file_path: str)declaresstr. This works at runtime sincepatch_ng.fromfileaccepts path-like objects, but it's inconsistent with the type annotation. Consider either updating the annotation inparse_patchto acceptstr | pathlib.Path, or wrapping withstr(original_patch_file)here.Option A: Fix at the call site
- parsed_patch = parse_patch(original_patch_file)+ parsed_patch = parse_patch(str(original_patch_file))dfetch/commands/format_patch.py (1)
153-162:_determine_target_patch_type— remove redundantstr()conversion.All three constants (
patch_ng.GIT,patch_ng.SVN,patch_ng.PLAIN) exist and are used consistently throughout the codebase. The isinstance-based dispatch is clean and the function is appropriately module-private.However, the
str()cast on line 162 is redundant. These constants are already strings—the function signature declares a-> strreturn type, and inconvert_patch_to(line 306),required_typeis compared directly topatch.typewith==, which works only because both are strings. Remove thestr()wrapper for clarity:return required_type.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pyproject.toml`:
- Line 100: The inline comment next to the dependency string 'setuptools;
python_version >= "3.12"' is stale because sphinxcontrib.details.directive is
being removed; either remove this setuptools marker from the docs extras if
nothing else requires pkg_resources, or update the comment to name the actual
consumer that needs setuptools (e.g., the specific Sphinx extension importing
pkg_resources). Locate the dependency string 'setuptools; python_version >=
"3.12"' in the pyproject.toml extras section and either delete the comment or
replace it with an accurate explanation of why setuptools is present.
🧹 Nitpick comments (2)
doc/_ext/scenario_directive.py (1)
81-96: Approach looks good — raw HTML replaces the removed sphinxcontrib-details-directive.The sequential
.. raw:: htmlblocks wrapping theliteralincludewill produce a valid collapsible<details>element in the final output. One minor concern:scenario_titleon line 84 is interpolated directly into raw HTML without escaping. If any Gherkin scenario title contains characters like<,>, or&, the HTML could break.Optional: escape the title
+import html ... - <summary><strong>Example</strong>: {scenario_title}</summary>+ <summary><strong>Example</strong>: {html.escape(scenario_title)}</summary>dfetch/commands/format_patch.py (1)
153-162:str()wrapping is redundant —patch_ng.GIT/SVN/PLAINare already strings.Very minor:
patch_ng.GIT,patch_ng.SVN, andpatch_ng.PLAINare string constants in patch-ng, soreturn str(required_type)could simply bereturn required_type. Not a problem, just a nit.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Update documentation
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
Chores