Allow multiple patches in manifest (fixes #897) #931
Conversation
WalkthroughThis PR changes the manifest Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Manifest
participant ProjectEntry
participant SubProject
participant FileSystem as "File System"
participant Metadata
User->>Manifest: load/parse dfetch.yaml
Manifest->>ProjectEntry: read project entry (patch field)
ProjectEntry->>ProjectEntry: normalize patch -> always_str_list -> list[str]
ProjectEntry->>Metadata: initialize metadata with patch list
User->>SubProject: fetch/update project
SubProject->>SubProject: for each patch in project.patch
loop apply each patch in order
SubProject->>FileSystem: load patch file (patch path)
FileSystem-->>SubProject: patch contents
SubProject->>FileSystem: apply patch to workspace
alt success
SubProject->>Metadata: record applied patch
else failure
SubProject->>Metadata: log warning for that patch
end
end
SubProject->>User: return metadata with applied patch list
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dfetch/manifest/project.py (1)
455-455: Fix type mismatch:_patchshould be[]not"".The
_patchattribute is nowlist[str], but this line assigns an empty string. This will cause a type error or runtime issues.🐛 Proposed fix
- recommendation._patch = "" # pylint: disable=protected-access + recommendation._patch = [] # pylint: disable=protected-access
🧹 Nitpick comments (4)
doc/_ext/patches/002-fix-options-sphinxcontrib.asciinema.patch (1)
30-31: Verify that all options innode["options"]exist inoption_spec.The coercion loop assumes every option key in
node["options"]has a corresponding entry inASCIINemaDirective.option_spec. If an unknown option is passed, this will raise aKeyError.Consider adding a guard:
♻️ Suggested defensive check
for option, value in node["options"].items(): - node["options"][option] = ASCIINemaDirective.option_spec[option](value) + if option in ASCIINemaDirective.option_spec: + node["options"][option] = ASCIINemaDirective.option_spec[option](value)dfetch/reporting/stdout_reporter.py (1)
38-40: Simplify the join expression.The generator expression is redundant since
metadata.patchis already an iterable of strings.♻️ Suggested simplification
logger.print_info_field( - " patch", ", ".join(patch for patch in metadata.patch) + " patch", ", ".join(metadata.patch) )dfetch/util/util.py (2)
139-141: Improve docstring clarity.The logic is correct, but the docstring "Ensure data is always a str list" could be more descriptive. Consider something like: "Convert a string or list of strings into a list of strings. Empty strings return an empty list."
📝 Suggested docstring improvement
def always_str_list(data: Union[str, list[str]]) -> list[str]: - """Ensure data is always a str list.""" + """Convert a string or list of strings into a list of strings. + + Args: + data: A string or list of strings. + + Returns: + A list of strings. Empty strings are converted to empty lists. + """ return data if not isinstance(data, str) else [data] if data else []
144-146: Fix docstring grammar and improve clarity.The docstring "Make list single str data is always a str list" is grammatically incorrect and unclear. The function converts single-element lists to strings, not the other way around.
📝 Suggested docstring improvement
def str_if_possible(data: list[str]) -> Union[str, list[str]]: - """Make list single str data is always a str list.""" + """Convert a single-element list to a string, otherwise keep as list. + + Args: + data: A list of strings. + + Returns: + A single string if the list has exactly one element, an empty string + if the list is empty, otherwise the original list. + """ return "" if not data else data[0] if len(data) == 1 else data
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
CHANGELOG.rstdfetch.yamldfetch/manifest/project.pydfetch/manifest/schema.pydfetch/project/metadata.pydfetch/project/subproject.pydfetch/reporting/stdout_reporter.pydfetch/util/util.pydoc/_ext/patches/001-autoformat-sphinxcontrib.asciinema.patchdoc/_ext/patches/002-fix-options-sphinxcontrib.asciinema.patchdoc/_ext/sphinxcontrib_asciinema/.dfetch_data.yamlfeatures/list-projects.featurefeatures/patch-after-fetch-git.featurefeatures/steps/git_steps.pytests/test_fuzzing.pytests/test_project_entry.py
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: spoorcc
Repo: dfetch-org/dfetch PR: 895
File: dfetch/commands/check.py:133-142
Timestamp: 2025-12-31T10:55:28.644Z
Learning: In dfetch, when determining ignored files for a project, the VCS detection should use the superproject's VCS type (where the manifest is located), not the fetched project's VCS type. The ignored files are then queried from the project's destination path using the superproject's VCS system.
🧬 Code graph analysis (5)
tests/test_fuzzing.py (2)
dfetch/manifest/project.py (1)
patch(413-415)dfetch/project/metadata.py (1)
patch(129-131)
dfetch/manifest/project.py (2)
dfetch/util/util.py (1)
always_str_list(139-141)dfetch/project/metadata.py (1)
patch(129-131)
features/steps/git_steps.py (4)
features/steps/manifest_steps.py (1)
generate_manifest(15-22)features/steps/generic_steps.py (1)
generate_file(107-115)dfetch/project/metadata.py (1)
path(134-142)dfetch/manifest/manifest.py (1)
path(245-247)
dfetch/project/metadata.py (3)
dfetch/manifest/project.py (3)
version(366-371)version(374-380)patch(413-415)dfetch/manifest/version.py (1)
Version(6-32)dfetch/util/util.py (2)
always_str_list(139-141)str_if_possible(144-146)
dfetch/reporting/stdout_reporter.py (3)
dfetch/log.py (1)
print_info_field(27-29)dfetch/manifest/project.py (1)
patch(413-415)dfetch/project/metadata.py (1)
patch(129-131)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: Publish Python distribution 📦 to TestPyPI
- GitHub Check: build / build (windows-latest)
- GitHub Check: build / build (macos-latest)
- GitHub Check: build / build (ubuntu-latest)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: run / run (windows-latest, 3.14)
- GitHub Check: run / run (macos-latest, 3.13)
- GitHub Check: run / run (windows-latest, 3.13)
- GitHub Check: run / run (macos-latest, 3.9)
- GitHub Check: run / run (windows-latest, 3.10)
- GitHub Check: run / run (ubuntu-latest, 3.14)
- GitHub Check: run / run (windows-latest, 3.12)
- GitHub Check: run / run (windows-latest, 3.11)
- GitHub Check: run / run (ubuntu-latest, 3.13)
- GitHub Check: run / run (windows-latest, 3.9)
- GitHub Check: run / run (macos-latest, 3.10)
- GitHub Check: run / test-cygwin
- GitHub Check: DevContainer Build & Test
🔇 Additional comments (15)
doc/_ext/patches/001-autoformat-sphinxcontrib.asciinema.patch (1)
1-152: LGTM - Valid auto-formatting patch.This patch applies consistent code style (double quotes, import sorting, line formatting) to the asciinema extension. The changes are purely cosmetic and align with Python formatting standards.
doc/_ext/patches/002-fix-options-sphinxcontrib.asciinema.patch (1)
12-13: Good catch fixing the typo.The correction from
terminallineheigthtoterminallineheight(and the corresponding JS name fix) is a valuable bug fix.CHANGELOG.rst (1)
9-9: LGTM - Changelog entry is properly documented.The entry correctly references issue #897 and is placed in the appropriate unreleased section.
tests/test_project_entry.py (1)
34-41: LGTM - Comprehensive test coverage for patch normalization.The test correctly verifies that both single string and list inputs are normalized to a list format.
dfetch/manifest/schema.py (1)
28-28: LGTM - Schema correctly extended for multi-patch support.The union type
SAFE_STR | Seq(SAFE_STR)maintains backward compatibility while enabling the new multi-patch functionality. Existing manifests with single patch strings will continue to work.features/list-projects.feature (1)
82-96: LGTM - Test scenario properly updated for multi-patch support.The step definition exists and correctly handles the multi-patch input. The scenario validates that comma-separated patches are properly displayed in the report output.
tests/test_fuzzing.py (1)
110-110: LGTM! Fuzzing strategy correctly extended for multi-patch support.The patch field strategy now correctly generates either a single string or a list of strings, aligning with the Union[str, list[str]] type. The min_size=1 constraint ensures non-empty lists are tested, which appears appropriate.
doc/_ext/sphinxcontrib_asciinema/.dfetch_data.yaml (1)
5-9: LGTM! Generated metadata correctly reflects multi-patch format.The metadata file correctly shows patches stored as a list, demonstrating the feature is working as intended. The updated hash and timestamp are expected for regenerated metadata.
dfetch.yaml (1)
23-25: LGTM! Manifest correctly uses multi-patch syntax.The patch field properly demonstrates the new list format, specifying two patch files to be applied sequentially. The YAML syntax is correct.
features/patch-after-fetch-git.feature (1)
78-123: LGTM! Comprehensive test for sequential multi-patch application.The new scenario effectively validates that multiple patches are applied in order. The test design is clever—applying two patches where the second reverses the first confirms both patches executed and in the correct sequence.
features/steps/git_steps.py (1)
150-199: LGTM! Test updated to validate multi-patch functionality.The test setup correctly demonstrates the new multi-patch feature:
- Manifest declares patch as a list with two entries
- Two patch files are generated with valid diff content
- The patches are applied sequentially via the update command
The opposing nature of the two patches (one changes "dfetch" to "patch", the other reverses it) effectively tests that both patches are applied in order.
dfetch/project/subproject.py (2)
131-143: LGTM! Multi-patch application logic is correct.The implementation properly:
- Iterates over all patches in the project
- Validates existence before applying
- Collects successfully applied patches
- Warns about missing patches without failing
- Records applied patches in metadata
148-157: LGTM! Patch application refactored to accept patch path.The method signature change enables per-patch application, with error messages and logging correctly referencing the specific patch being processed.
dfetch/manifest/project.py (1)
281-281: LGTM! Patch field correctly updated to support multiple patches.The implementation properly:
- Imports the
always_str_listutility- Updates the TypedDict to accept
Union[str, list[str]]- Coerces input to a list at initialization
- Returns
list[str]from the propertyThis allows backward compatibility (single string) while enabling multi-patch support.
Also applies to: 292-292, 320-320, 413-415
dfetch/project/metadata.py (1)
5-5: LGTM! Metadata correctly updated for multi-patch support.The implementation properly:
- Adds necessary imports (
Union,always_str_list,str_if_possible)- Updates type annotations to
Union[str, list[str]]for input andlist[str]for storage- Coerces patch data to a list at initialization and in
fetched()- Uses
str_if_possible()during serialization to maintain backward compatibility (single patches serialize as strings)The approach ensures that metadata can read old single-patch files while supporting new multi-patch functionality.
Also applies to: 12-12, 30-30, 55-56, 80-87, 129-131, 169-169
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
dfetch/util/util.py (1)
144-146: Clarify the docstring.The docstring "Make list single str data is always a str list." is grammatically unclear and doesn't accurately describe the function's behavior.
📝 Suggested docstring improvement
def str_if_possible(data: list[str]) -> Union[str, list[str]]: - """Make list single str data is always a str list.""" + """Convert a single-element list to a string, or return empty string if empty.""" return "" if not data else data[0] if len(data) == 1 else datadfetch/manifest/project.py (1)
462-477: Consider usingstr_if_possiblefor cleaner YAML serialization.The
as_yamlmethod currently serializespatchas alist[str]directly. For better user experience, consider usingstr_if_possible(self._patch)to serialize single-item lists as strings, similar to howMetadata.dump()handles it (Line 169 in dfetch/project/metadata.py).This would produce cleaner YAML output:
patch: "single.patch"instead ofpatch: ["single.patch"]patch: ["one.patch", "two.patch"]for multiple patches♻️ Proposed refactor
Import the utility at the top of the file:
from dfetch.manifest.remote import Remote from dfetch.manifest.version import Version -from dfetch.util.util import always_str_list +from dfetch.util.util import always_str_list, str_if_possibleThen update the serialization:
def as_yaml(self) -> dict[str, Union[str, list[str]]]: """Get this project as yaml dictionary.""" yamldata = { "name": self._name, "revision": self._revision, "remote": self._remote, "src": self._src, "dst": self._dst if self._dst != self._name else None, "url": self._url, - "patch": self._patch, + "patch": str_if_possible(self._patch), "branch": self._branch, "tag": self._tag, "repo-path": self._repo_path, "vcs": self._vcs, } return {k: v for k, v in yamldata.items() if v}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
CHANGELOG.rstdfetch.yamldfetch/manifest/manifest.pydfetch/manifest/project.pydfetch/manifest/schema.pydfetch/project/metadata.pydfetch/project/subproject.pydfetch/reporting/stdout_reporter.pydfetch/util/util.pydoc/_ext/patches/001-autoformat-sphinxcontrib.asciinema.patchdoc/_ext/patches/002-fix-options-sphinxcontrib.asciinema.patchdoc/_ext/sphinxcontrib_asciinema/.dfetch_data.yamlfeatures/list-projects.featurefeatures/patch-after-fetch-git.featurefeatures/steps/git_steps.pytests/test_fuzzing.pytests/test_project_entry.py
🚧 Files skipped from review as they are similar to previous changes (6)
- features/list-projects.feature
- dfetch/project/subproject.py
- dfetch/reporting/stdout_reporter.py
- tests/test_fuzzing.py
- dfetch.yaml
- doc/_ext/sphinxcontrib_asciinema/.dfetch_data.yaml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-31T10:55:28.644Z
Learnt from: spoorcc
Repo: dfetch-org/dfetch PR: 895
File: dfetch/commands/check.py:133-142
Timestamp: 2025-12-31T10:55:28.644Z
Learning: In dfetch, when determining ignored files for a project, the VCS detection should use the superproject's VCS type (where the manifest is located), not the fetched project's VCS type. The ignored files are then queried from the project's destination path using the superproject's VCS system.
Applied to files:
dfetch/manifest/manifest.py
🧬 Code graph analysis (5)
features/steps/git_steps.py (2)
features/steps/manifest_steps.py (1)
generate_manifest(15-22)features/steps/generic_steps.py (1)
generate_file(107-115)
dfetch/manifest/project.py (3)
dfetch/util/util.py (1)
always_str_list(139-141)dfetch/project/metadata.py (2)
patch(129-131)remote_url(110-112)dfetch/manifest/remote.py (1)
as_yaml(91-98)
dfetch/project/metadata.py (3)
dfetch/manifest/project.py (4)
ProjectEntry(305-478)version(366-371)version(374-380)patch(413-415)dfetch/manifest/version.py (1)
Version(6-32)dfetch/util/util.py (2)
always_str_list(139-141)str_if_possible(144-146)
tests/test_project_entry.py (1)
dfetch/manifest/project.py (2)
ProjectEntry(305-478)patch(413-415)
dfetch/manifest/manifest.py (1)
dfetch/manifest/project.py (3)
ProjectEntry(305-478)from_yaml(331-350)as_yaml(462-478)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: Publish Python distribution 📦 to TestPyPI
- GitHub Check: build / build (macos-latest)
- GitHub Check: build / build (ubuntu-latest)
- GitHub Check: build / build (windows-latest)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: run / run (windows-latest, 3.11)
- GitHub Check: run / run (windows-latest, 3.14)
- GitHub Check: run / run (macos-latest, 3.12)
- GitHub Check: run / run (windows-latest, 3.12)
- GitHub Check: run / run (windows-latest, 3.13)
- GitHub Check: run / run (macos-latest, 3.14)
- GitHub Check: run / run (windows-latest, 3.9)
- GitHub Check: run / run (windows-latest, 3.10)
- GitHub Check: run / run (macos-latest, 3.9)
- GitHub Check: run / test-cygwin
- GitHub Check: test / test
- GitHub Check: DevContainer Build & Test
🔇 Additional comments (24)
features/steps/git_steps.py (1)
150-199: LGTM! Sequential patch application test setup looks correct.The test setup properly demonstrates multiple patch support. The two patches apply inverse changes (first changes "dfetch" → "patch", second changes "patch" → "dfetch"), which is an effective way to verify sequential patch application behavior.
CHANGELOG.rst (1)
9-9: LGTM! Changelog entry properly formatted.The changelog entry correctly documents the new multi-patch feature and references the corresponding issue.
dfetch/manifest/schema.py (1)
28-28: LGTM! Schema correctly updated for multi-patch support.The patch field now properly accepts either a single string or a sequence of strings, enabling flexible manifest definitions.
tests/test_project_entry.py (1)
34-40: LGTM! Test coverage for patch normalization is correct.The test properly verifies that both string and list inputs for the patch field are normalized to a list format, ensuring consistent behavior regardless of input format.
dfetch/util/util.py (1)
139-141: No action required—empty string handling is intentional.The function correctly returns an empty list
[]for empty strings, which is consistent with the design: the default value for missing patch fields is also[], making the behavior semantically correct. An empty patch field should not create a patch entry, so treating""as no patch is the intended behavior.features/patch-after-fetch-git.feature (1)
78-123: LGTM! Test scenario correctly validates sequential patch application.The new scenario properly tests that multiple patches are applied in order. The first patch modifies the content, and the second patch further modifies it, with the final state correctly reflecting both transformations.
doc/_ext/patches/001-autoformat-sphinxcontrib.asciinema.patch (1)
1-152: LGTM! Formatting and code style improvements.This patch applies automated formatting changes to the asciinema Sphinx extension, including import reordering, consistent quote styles, and improved line spacing. These changes improve code consistency without altering functionality.
dfetch/manifest/project.py (5)
281-281: LGTM! Import supports patch normalization.The
always_str_listutility is correctly imported to normalize patch values from either string or list format to a consistent list representation.
292-292: LGTM! Type correctly supports single or multiple patches.The TypedDict entry for patch now accepts both
strandlist[str], maintaining backward compatibility while enabling the new multi-patch feature.
320-320: LGTM! Patch normalization is correctly implemented.The patch field is properly normalized to a list using
always_str_list, ensuring consistent internal representation regardless of whether a single string or list is provided in the manifest.
413-415: LGTM! Patch property correctly returns list.The patch property now returns
list[str], consistent with the internal representation and enabling support for multiple patches.
455-455: LGTM! Recommendation correctly clears patches.The
as_recommendationmethod now sets patch to an empty list[]instead of an empty string, consistent with the new list-based representation.doc/_ext/patches/002-fix-options-sphinxcontrib.asciinema.patch (5)
39-56: LGTM! Option parsers correctly validate boolean and numeric inputs.The
bool_parseandbool_or_positive_inthelper functions provide proper validation for directive options, ensuring type safety and clear error messages for invalid values.
62-79: LGTM! Option validators properly enforce types.The
option_specupdates correctly apply the new boolean and numeric parsers to relevant options, improving validation and type safety for the directive.
13-13: LGTM! Typo corrected.Fixed spelling:
terminallineheigth→terminallineheight
19-28: LGTM! Options list expanded for proper handling.The
options_rawlist now includes additional options that should be rendered without quotes in the HTML output, correctly supporting the expanded option set.
30-31: No KeyError risk—options are validated by Sphinx during directive processing.The code at lines 30-31 safely accesses
option_spec[option]because Sphinx's directive framework validates all options againstoption_specduring directive processing. Only options defined inoption_specare added tonode["options"], making the loop safe from KeyError.Likely an incorrect or invalid review comment.
dfetch/project/metadata.py (6)
5-5: LGTM! Imports support multi-patch functionality.The additional imports (
Optional,Union,always_str_list,str_if_possible) are correctly added to support the new patch handling logic.Also applies to: 12-12
30-30: LGTM! Options type correctly supports both forms.The
patchfield inOptionsTypedDict now acceptsUnion[str, list[str]], consistent with the manifest schema changes.
54-56: LGTM! Patch normalization with helpful context.The initialization correctly normalizes patch to
list[str]usingalways_str_list, and the comment appropriately notes the historical context of single-patch support.
80-87: LGTM! Fetched method correctly handles optional patch list.The
fetchedmethod signature now acceptsOptional[list[str]]for patches and properly defaults to an empty list when None is provided.
129-129: LGTM! Patch property returns list.The patch property correctly returns
list[str], consistent with the internal representation.
169-169: LGTM! Serialization uses str_if_possible for clean output.The
dumpmethod correctly usesstr_if_possibleto serialize patches, ensuring single-item lists are written as strings for cleaner YAML output while maintaining support for multiple patches.dfetch/manifest/manifest.py (1)
305-307: LGTM! Serialization typing correctly supports list-valued fields.The updated type annotations for
projectsandproject_yamlcorrectly reflect that project entries can now contain list values (e.g., for multiple patches). This aligns withProjectEntry.as_yaml()return type and enables proper serialization.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @doc/_ext/patches/002-fix-options-sphinxcontrib.asciinema.patch:
- Around line 30-31: The loop that transforms node["options"] can raise KeyError
when an option key is absent from ASCIINemaDirective.option_spec; update the
loop in the patch to iterate safely (e.g., over list(node["options"].items()) or
the keys copy) and for each option check membership in
ASCIINemaDirective.option_spec (or use option_spec.get(option, identity)) before
calling the converter; if the option is unknown, skip it or leave its value
unchanged to avoid KeyError while still applying conversions for known options.
🧹 Nitpick comments (2)
dfetch/project/subproject.py (1)
135-135: Consider usingappend()for better readability.The
+=operator with a single-element list is less idiomatic thanappend()in Python.♻️ Suggested refactor
- applied_patches += [patch] + applied_patches.append(patch)dfetch/manifest/project.py (1)
462-478: Consider usingstr_if_possiblefor consistent YAML output.In
dfetch/project/metadata.pyline 169,str_if_possible(self.patch)is used to serialize patches, converting single-element lists back to strings for cleaner YAML. However,as_yaml()here returnsself._patchdirectly as a list, which may result in inconsistent YAML output between metadata files and manifests.♻️ Optional consistency fix
+from dfetch.util.util import always_str_list, str_if_possible -from dfetch.util.util import always_str_list # ... in as_yaml(): yamldata = { "name": self._name, "revision": self._revision, "remote": self._remote, "src": self._src, "dst": self._dst if self._dst != self._name else None, "url": self._url, - "patch": self._patch, + "patch": str_if_possible(self._patch), "branch": self._branch, "tag": self._tag, "repo-path": self._repo_path, "vcs": self._vcs, }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
CHANGELOG.rstdfetch.yamldfetch/manifest/manifest.pydfetch/manifest/project.pydfetch/manifest/schema.pydfetch/project/metadata.pydfetch/project/subproject.pydfetch/reporting/stdout_reporter.pydfetch/util/util.pydoc/_ext/patches/001-autoformat-sphinxcontrib.asciinema.patchdoc/_ext/patches/002-fix-options-sphinxcontrib.asciinema.patchdoc/_ext/sphinxcontrib_asciinema/.dfetch_data.yamlfeatures/list-projects.featurefeatures/patch-after-fetch-git.featurefeatures/steps/git_steps.pytests/test_fuzzing.pytests/test_project_entry.py
🚧 Files skipped from review as they are similar to previous changes (6)
- CHANGELOG.rst
- dfetch.yaml
- dfetch/manifest/schema.py
- dfetch/util/util.py
- tests/test_fuzzing.py
- features/steps/git_steps.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-31T10:55:28.644Z
Learnt from: spoorcc
Repo: dfetch-org/dfetch PR: 895
File: dfetch/commands/check.py:133-142
Timestamp: 2025-12-31T10:55:28.644Z
Learning: In dfetch, when determining ignored files for a project, the VCS detection should use the superproject's VCS type (where the manifest is located), not the fetched project's VCS type. The ignored files are then queried from the project's destination path using the superproject's VCS system.
Applied to files:
dfetch/manifest/manifest.py
🧬 Code graph analysis (5)
dfetch/manifest/project.py (2)
dfetch/util/util.py (1)
always_str_list(139-148)dfetch/project/metadata.py (2)
patch(129-131)remote_url(110-112)
dfetch/manifest/manifest.py (2)
dfetch/manifest/project.py (3)
ProjectEntry(305-478)from_yaml(331-350)as_yaml(462-478)dfetch/manifest/remote.py (2)
from_yaml(44-56)as_yaml(91-98)
dfetch/reporting/stdout_reporter.py (3)
dfetch/log.py (1)
print_info_field(27-29)dfetch/manifest/project.py (1)
patch(413-415)dfetch/project/metadata.py (1)
patch(129-131)
dfetch/project/metadata.py (1)
dfetch/util/util.py (2)
always_str_list(139-148)str_if_possible(151-161)
tests/test_project_entry.py (2)
dfetch/manifest/project.py (1)
patch(413-415)dfetch/project/metadata.py (1)
patch(129-131)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: build / build (windows-latest)
- GitHub Check: build / build (ubuntu-latest)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: run / run (windows-latest, 3.13)
- GitHub Check: run / run (windows-latest, 3.12)
- GitHub Check: run / run (windows-latest, 3.14)
- GitHub Check: run / run (macos-latest, 3.10)
- GitHub Check: run / run (windows-latest, 3.10)
- GitHub Check: run / run (windows-latest, 3.11)
- GitHub Check: run / run (windows-latest, 3.9)
- GitHub Check: run / test-cygwin
- GitHub Check: test / test
- GitHub Check: DevContainer Build & Test
🔇 Additional comments (14)
dfetch/reporting/stdout_reporter.py (1)
38-38: LGTM! Patch list formatting is correct.The comma-separated join correctly formats the patch list for display. Empty lists are handled appropriately (will display as "" via
print_info_field).doc/_ext/sphinxcontrib_asciinema/.dfetch_data.yaml (1)
5-9: Generated metadata reflects multi-patch support correctly.The updated metadata correctly represents multiple patches as a list. As this is a generated file, the changes align with the broader PR modifications.
features/list-projects.feature (1)
82-96: LGTM! Feature test correctly validates multi-patch reporting.The scenario appropriately tests the new multi-patch functionality, with expected output matching the comma-separated format implemented in the stdout reporter.
tests/test_project_entry.py (1)
34-40: LGTM! Comprehensive test coverage for patch normalization.The test correctly verifies that both string and list inputs are normalized to a list representation, which aligns with the
always_str_listutility behavior documented in the PR.dfetch/project/subproject.py (2)
131-143: LGTM! Multi-patch application logic is correct.The implementation properly:
- Iterates over multiple patches sequentially
- Tracks successfully applied patches
- Warns about missing patch files without failing
- Stores the applied patch list in metadata
The fail-fast behavior (RuntimeError on patch application failure) is appropriate, ensuring incomplete patch sets don't result in partially-patched code.
148-157: LGTM! Method signature change enables per-patch application.The updated signature correctly accepts a specific patch path parameter, allowing the method to be called independently for each patch in the list. Error handling and logging appropriately reference the specific patch being applied.
features/patch-after-fetch-git.feature (1)
78-123: LGTM! Good test coverage for multi-patch functionality.The scenario correctly validates sequential patch application by using two patches that compose in order—first modifying the content, then reverting it. This effectively tests that patches are applied in the specified order.
doc/_ext/patches/001-autoformat-sphinxcontrib.asciinema.patch (1)
1-152: LGTM! Autoformatting patch for sphinxcontrib_asciinema.This patch contains consistent style normalization (single to double quotes, import reordering, line break formatting) that appears to be auto-generated formatting changes. It serves as a good example patch file for demonstrating the multi-patch feature.
dfetch/project/metadata.py (3)
54-56: LGTM! Clean backward-compatible patch normalization.Using
always_str_listensures both legacy single-string patches and new list-based patches are consistently stored aslist[str]. The comment explaining the historical context is helpful for maintainability.
80-87: LGTM! Correct signature update for multi-patch support.The
fetched()method correctly accepts an optional list of patches and defaults to an empty list whenNoneis passed.
169-169: LGTM! Smart serialization preserves backward compatibility.Using
str_if_possibleensures single-patch configurations remain as simple strings in the YAML output, maintaining backward compatibility with existing manifest consumers.doc/_ext/patches/002-fix-options-sphinxcontrib.asciinema.patch (1)
39-48: Consider using standard boolean parsing for consistency.The
bool_parsefunction returns lowercase string"true"or"false"rather than Python boolean values. This may be intentional for JavaScript compatibility in the HTML output, but worth verifying this is the expected behavior.dfetch/manifest/project.py (1)
320-320: LGTM! Consistent patch normalization.Using
always_str_listensures backward compatibility with legacy single-string patches while normalizing to the newlist[str]internal representation.dfetch/manifest/manifest.py (1)
305-307: LGTM! Type annotations updated consistently.The
projectslist andproject_yamlvariable types correctly reflect the broadened Union type to accommodate list-based patch values.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
dfetch/manifest/project.py (1)
333-333: Consider more precise type annotations.The type annotations on lines 333 and 462 use
Union[str, list[str]]for all dict values, but only thepatchfield actually has this dual nature. Most fields are plainstr.While this broad typing is pragmatic for YAML parsing (and the
# type: ignoreon line 346 acknowledges the limitation), you could improve precision with a dedicated YAML representation type that explicitly shows which fields accept which types.This is a minor refinement—the current implementation works correctly.
Also applies to: 462-462
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
CHANGELOG.rstdfetch.yamldfetch/__main__.pydfetch/manifest/manifest.pydfetch/manifest/project.pydfetch/manifest/schema.pydfetch/project/metadata.pydfetch/project/subproject.pydfetch/reporting/stdout_reporter.pydfetch/util/util.pydoc/_ext/patches/001-autoformat-sphinxcontrib.asciinema.patchdoc/_ext/patches/002-fix-options-sphinxcontrib.asciinema.patchdoc/_ext/sphinxcontrib_asciinema/.dfetch_data.yamlfeatures/list-projects.featurefeatures/patch-after-fetch-git.featurefeatures/steps/git_steps.pytests/test_fuzzing.pytests/test_project_entry.py
🚧 Files skipped from review as they are similar to previous changes (7)
- features/steps/git_steps.py
- dfetch.yaml
- CHANGELOG.rst
- doc/_ext/sphinxcontrib_asciinema/.dfetch_data.yaml
- tests/test_fuzzing.py
- dfetch/reporting/stdout_reporter.py
- features/list-projects.feature
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-31T10:55:28.644Z
Learnt from: spoorcc
Repo: dfetch-org/dfetch PR: 895
File: dfetch/commands/check.py:133-142
Timestamp: 2025-12-31T10:55:28.644Z
Learning: In dfetch, when determining ignored files for a project, the VCS detection should use the superproject's VCS type (where the manifest is located), not the fetched project's VCS type. The ignored files are then queried from the project's destination path using the superproject's VCS system.
Applied to files:
dfetch/manifest/manifest.py
📚 Learning: 2026-01-01T11:05:43.761Z
Learnt from: ben-edna
Repo: dfetch-org/dfetch PR: 898
File: script/package.py:201-205
Timestamp: 2026-01-01T11:05:43.761Z
Learning: In the dfetch project's script/package.py, prefer explicit failures ("crash and burn") over silently handling missing configuration values, ensuring that builds fail if required metadata (e.g., URLs in pyproject.toml) is not properly configured.
Applied to files:
dfetch/manifest/manifest.py
🧬 Code graph analysis (5)
dfetch/project/subproject.py (3)
dfetch/manifest/project.py (1)
patch(413-415)dfetch/project/metadata.py (4)
patch(129-131)path(134-142)fetched(80-87)dump(159-175)dfetch/util/util.py (1)
hash_directory(108-124)
dfetch/project/metadata.py (3)
dfetch/manifest/project.py (4)
ProjectEntry(305-478)version(366-371)version(374-380)patch(413-415)dfetch/manifest/version.py (1)
Version(6-32)dfetch/util/util.py (2)
always_str_list(139-148)str_if_possible(151-161)
dfetch/manifest/manifest.py (1)
dfetch/manifest/project.py (2)
ProjectEntry(305-478)as_yaml(462-478)
tests/test_project_entry.py (2)
dfetch/manifest/project.py (1)
patch(413-415)dfetch/project/metadata.py (1)
patch(129-131)
dfetch/manifest/project.py (2)
dfetch/util/util.py (2)
always_str_list(139-148)str_if_possible(151-161)dfetch/project/metadata.py (1)
patch(129-131)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: Publish Python distribution 📦 to TestPyPI
- GitHub Check: build / build (windows-latest)
- GitHub Check: build / build (macos-latest)
- GitHub Check: build / build (ubuntu-latest)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: run / run (ubuntu-latest, 3.9)
- GitHub Check: run / run (windows-latest, 3.11)
- GitHub Check: run / run (windows-latest, 3.14)
- GitHub Check: run / run (windows-latest, 3.12)
- GitHub Check: run / run (windows-latest, 3.13)
- GitHub Check: run / run (windows-latest, 3.9)
- GitHub Check: run / run (windows-latest, 3.10)
- GitHub Check: run / run (macos-latest, 3.13)
- GitHub Check: run / test-cygwin
- GitHub Check: test / test
- GitHub Check: DevContainer Build & Test
🔇 Additional comments (20)
doc/_ext/patches/001-autoformat-sphinxcontrib.asciinema.patch (1)
1-152: LGTM: Valid formatting patch.This patch applies consistent formatting changes (import ordering, quote style, multiline formatting) to the Asciinema extension. The patch format is valid and the changes are purely stylistic with no functional impact.
doc/_ext/patches/002-fix-options-sphinxcontrib.asciinema.patch (2)
8-28: LGTM: Option mapping expansion looks correct.The patch properly expands the option mappings and corrects the typo from "terminallineheigth" to "terminallineheight". The expanded
options_rawlist appropriately includes options that require raw value handling (booleans, integers, arrays).
39-82: LGTM: Parser functions and option_spec updates are well-structured.The new
bool_parseandbool_or_positive_inthelper functions provide appropriate validation for boolean and numeric option values. The updatedoption_speccorrectly applies these parsers to the relevant options (autoplay, preload, loop, pauseOnMarkers).dfetch/manifest/schema.py (1)
28-28: LGTM: Schema change correctly enables multiple patches.The schema update from
SAFE_STRtoSAFE_STR | Seq(SAFE_STR)properly allows the patch field to accept either a single string or a list of strings. This is backward-compatible and correctly implements the PR's core objective. The union syntax is appropriate for strictyaml 1.7.3.tests/test_project_entry.py (1)
34-40: LGTM: Tests correctly verify patch normalization behavior.The updated test properly verifies both input scenarios:
- String input
"diff.patch"is normalized to["diff.patch"]- List input
["diff.patch"]is preserved as["diff.patch"]This confirms the backward-compatible normalization behavior and validates that the
patchproperty consistently returnslist[str].dfetch/__main__.py (1)
68-68: TheTypeErroraddition is justified and correct.The new validation code in
manifest.py:168raisesTypeErrorwhen a project name is not a string. This validation executes during manifest loading within theargs.func(args)call at line 67, so the exception handler at line 68 correctly needs to catchTypeError. No changes required.dfetch/util/util.py (2)
139-148: LGTM!The utility function correctly handles conversion of strings or lists of strings to a normalized list format. The edge case of empty strings converting to empty lists is handled appropriately.
151-161: LGTM!The function correctly converts single-element lists to strings for cleaner YAML serialization while preserving multi-element lists. The logic is clean and handles all cases appropriately.
dfetch/project/subproject.py (2)
131-142: LGTM!The multi-patch application logic is well-structured:
- Patches are applied sequentially in order
- Non-existent patches are logged and skipped gracefully
- Only successfully applied patches are tracked in metadata
- If any patch fails to apply (via RuntimeError), the metadata won't be updated, which is the correct failure behavior
148-157: LGTM!The refactored
apply_patchmethod is now more flexible and testable by accepting a patch path as a parameter rather than accessing a class-level reference. Error messages and logging correctly reference the specific patch being processed.features/patch-after-fetch-git.feature (1)
78-123: LGTM!The new test scenario effectively validates multi-patch functionality:
- Demonstrates YAML list syntax for multiple patches
- Verifies sequential application order (patch 1 changes line, patch 2 reverts it)
- Confirms final state matches the cumulative effect of all patches
This provides good coverage for the new multi-patch feature.
dfetch/project/metadata.py (6)
5-5: LGTM!The added imports provide the necessary types and utilities for handling patches as either strings or lists.
Also applies to: 12-12
30-30: LGTM!The
Optionstype definition correctly accepts patch data in either string or list format, matching the flexibility of the YAML input.
54-56: LGTM!The initialization properly normalizes patch data to a list using
always_str_list. The comment provides helpful context about the evolution from single to multiple patches.
80-87: LGTM!The
fetchedmethod signature correctly accepts an optional list of patches and defaults to an empty list when None. This aligns with the calling code insubproject.pythat passes the list of successfully applied patches.
129-131: LGTM!The property correctly returns
list[str], consistent with the internal storage and the broader patch handling changes across the codebase.
169-169: LGTM!The metadata serialization uses
str_if_possibleto produce cleaner YAML files:
- Single patches are saved as strings
- Multiple patches are saved as lists
This maintains readability while supporting the new multi-patch feature.
dfetch/manifest/manifest.py (1)
167-170: Good defensive validation added.The explicit type check for
project["name"]prevents invalid input (e.g., a list) from being silently converted to a malformed string representation. This addresses the concern raised in previous reviews and provides clear error messaging.dfetch/manifest/project.py (2)
281-281: Well-structured implementation of multi-patch support.The changes effectively enable multiple patches while maintaining backward compatibility:
always_str_listnormalizes patch input (string or list) to consistentlist[str]internal storagestr_if_possibleserializes back to string for single-patch case, preserving existing YAML format- All related field updates are consistent (initialization, property accessor, recommendation, serialization)
The helper functions provide a clean abstraction for bidirectional conversion.
Also applies to: 320-320, 413-415, 455-455, 471-471
413-415: All callers correctly handle thelist[str]return type.The
patchproperty's change fromstrtolist[str]has been properly integrated across the codebase. Verification shows all call sites handle the new return type correctly:
- Iteration with
forloops (subproject.py)- List joining (stdout_reporter.py)
- Serialization via
str_if_possible()for cleaner YAML (metadata.py)- Deserialization via
always_str_list()for type conversionNo missed call sites found.
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.