Improve minor various patch issues #942
Conversation
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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 |
spoorcc
added
bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @CHANGELOG.rst:
- Line 12: Fix the typo in the changelog entry by replacing the misspelled word
"independeny" with "independent" in the line "* Make patch path in metadata
platform independeny (#937)" so the entry reads "* Make patch path in metadata
platform independent (#937)".
In @dfetch/project/subproject.py:
- Around line 143-168: _subproject._apply_patches currently uses
Path(".").resolve() which assumes CWD is manifest dir; modify SubProject to
accept and store the manifest/root directory (e.g., self._manifest_dir) via its
constructor and update SuperProject where SubProject instances are created to
pass self._root_directory (or equivalent) to the SubProject constructor; then
change _apply_patches to use the stored manifest dir (self._manifest_dir)
instead of Path(".").resolve() so patch path validation and normalization work
correctly regardless of current working directory.
In @dfetch/vcs/patch.py:
- Around line 62-81: In apply_patch, the fallback path decodes the raw file into
text then re-encodes it to bytes before calling patch_ng.fromstring; remove the
unnecessary .encode("utf-8") so that patch_text is the string returned by
patch_ng.decode_text(...) and pass that string directly to
patch_ng.fromstring(...) (adjust the patch_set assignment and keep the existing
logger.warning and error checks unchanged).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CHANGELOG.rstdfetch/manifest/project.pydfetch/project/subproject.pydfetch/vcs/patch.pyfeatures/patch-after-fetch-git.featurefeatures/steps/generic_steps.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-02T22:46:53.305Z
Learnt from: spoorcc
Repo: dfetch-org/dfetch PR: 904
File: script/package.py:10-18
Timestamp: 2026-01-02T22:46:53.305Z
Learning: In dfetch's script/package.py, importing dfetch.__version__ directly is acceptable and intentional; if dfetch is not installed in the environment, the packaging job should fail explicitly rather than handling the import error gracefully.
Applied to files:
dfetch/project/subproject.py
🧬 Code graph analysis (2)
dfetch/project/subproject.py (3)
dfetch/manifest/project.py (2)
vcs(435-437)patch(415-417)dfetch/project/metadata.py (1)
patch(129-131)dfetch/vcs/patch.py (1)
apply_patch(62-80)
dfetch/vcs/patch.py (1)
dfetch/log.py (1)
get_logger(60-63)
⏰ 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). (15)
- 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.13)
- GitHub Check: run / run (macos-latest, 3.12)
- GitHub Check: run / run (windows-latest, 3.12)
- GitHub Check: run / run (windows-latest, 3.11)
- GitHub Check: run / run (windows-latest, 3.14)
- 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 / test-cygwin
- GitHub Check: test / test
- GitHub Check: DevContainer Build & Test
🔇 Additional comments (9)
dfetch/manifest/project.py (1)
228-230: LGTM! Clear documentation for cross-platform patch handling.The updated documentation appropriately specifies UTF-8 encoding requirements and encourages forward slashes for cross-platform compatibility, which aligns well with the implementation changes.
dfetch/vcs/patch.py (1)
11-13: LGTM! Proper logging setup.The logger initialization follows the project's logging pattern.
dfetch/project/subproject.py (2)
8-8: LGTM! Necessary import for path handling.
20-20: LGTM! Correct refactoring to use centralized patch application.The import change properly delegates patch application to the dedicated utility function.
features/patch-after-fetch-git.feature (3)
124-132: LGTM! Enhanced test coverage for multiple patches.The updated output expectations correctly verify that each patch is applied and logged individually.
134-176: LGTM! Comprehensive test for encoding fallback.This scenario effectively tests the UTF-8 encoding fallback mechanism, including the error message from patch_ng and the subsequent warning and successful application.
177-199: LGTM! Good test coverage for security boundary.This scenario properly validates that patches outside the manifest directory are skipped, which is important for preventing path traversal issues.
features/steps/generic_steps.py (2)
107-107: LGTM! Encoding parameter added correctly.The encoding parameter is properly added to
generate_filewith a sensible default, maintaining backward compatibility while enabling tests to generate files with different encodings (e.g., UTF-16 patches mentioned in the PR objectives).Also applies to: 113-113
204-207: LGTM! Dual decorator pattern correctly implements encoding support.The implementation properly uses Behave's multiple decorator pattern to support both the original step format and a new format with explicit encoding. The encoding parameter is correctly threaded through to
generate_file, enabling tests to verify patch handling with different encodings (addressing the UTF-16 encoding error mentioned in the PR objectives).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @dfetch/project/subproject.py:
- Around line 143-166: The _apply_patches method currently lets apply_patch
raise RuntimeError and abort processing; catch RuntimeError around the
apply_patch(normalized_patch_path, root=self.local_path) call, log the failure
via self._log_project including the error message and the patch name, and break
or continue as desired (for tests expect graceful failure: stop applying further
patches but return the list of already applied_patches so metadata can be
updated); ensure applied_patches is returned after handling the exception so
partial progress is persisted.
🧹 Nitpick comments (1)
dfetch/project/subproject.py (1)
5-8: Minor: Redundant pathlib imports.Line 5 imports
pathlibmodule and line 8 importsPathfrompathlib. Consider keeping only the specific import.♻️ Suggested cleanup
import fnmatch import os -import pathlib from abc import ABC, abstractmethod from collections.abc import Sequence from pathlib import PathThen update line 304 and 322 to use
Path(...)instead ofpathlib.Path(...).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CHANGELOG.rstdfetch/manifest/project.pydfetch/project/subproject.pyfeatures/patch-after-fetch-git.featurefeatures/steps/generic_steps.py
✅ Files skipped from review due to trivial changes (1)
- dfetch/manifest/project.py
🚧 Files skipped from review as they are similar to previous changes (2)
- features/steps/generic_steps.py
- CHANGELOG.rst
🧰 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/project/subproject.py
📚 Learning: 2026-01-02T22:46:53.305Z
Learnt from: spoorcc
Repo: dfetch-org/dfetch PR: 904
File: script/package.py:10-18
Timestamp: 2026-01-02T22:46:53.305Z
Learning: In dfetch's script/package.py, importing dfetch.__version__ directly is acceptable and intentional; if dfetch is not installed in the environment, the packaging job should fail explicitly rather than handling the import error gracefully.
Applied to files:
dfetch/project/subproject.py
🧬 Code graph analysis (1)
dfetch/project/subproject.py (3)
dfetch/manifest/project.py (2)
vcs(435-437)patch(415-417)dfetch/project/metadata.py (1)
patch(129-131)dfetch/vcs/patch.py (1)
apply_patch(62-80)
⏰ 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). (15)
- GitHub Check: build / build (ubuntu-latest)
- GitHub Check: build / build (windows-latest)
- GitHub Check: build / build (macos-latest)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: run / run (macos-latest, 3.9)
- GitHub Check: run / run (windows-latest, 3.12)
- GitHub Check: run / run (windows-latest, 3.9)
- GitHub Check: run / run (windows-latest, 3.14)
- GitHub Check: run / run (windows-latest, 3.10)
- GitHub Check: run / run (windows-latest, 3.13)
- GitHub Check: run / run (macos-latest, 3.10)
- GitHub Check: run / run (windows-latest, 3.11)
- GitHub Check: run / test-cygwin
- GitHub Check: DevContainer Build & Test
- GitHub Check: test / test
🔇 Additional comments (4)
features/patch-after-fetch-git.feature (3)
124-132: LGTM!Good addition of output verification for the multiple patches scenario, confirming that each patch application is logged individually.
134-175: Well-structured encoding fallback test.This scenario properly tests the UTF-16 fallback behavior, validating that the warning message is displayed while still successfully applying the patch.
194-199: No issue here. The test infrastructure uses regex-based path normalization (line 185 offeatures/steps/generic_steps.py) to replace absolute paths matching/tmp/[\w_]+with the literal/some/path. Sincecwd = Path(".").resolve()insubproject.pyproduces an absolute path like/tmp/test_xyz..., it gets normalized to/some/pathin both the actual and expected output before comparison. The test will pass.Likely an incorrect or invalid review comment.
dfetch/project/subproject.py (1)
145-155: Path validation logic is sound for platform independence.The approach of resolving paths relative to
cwdand usingas_posix()for normalization correctly addresses the platform-independent path issue from #937. Therelative_to()check properly prevents path traversal outside the manifest directory.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
dfetch/project/subproject.py (1)
142-165: Excellent refactoring! Platform-independent patch paths now properly handled.The new
_apply_patchesmethod successfully addresses issue #937 by normalizing patch paths to POSIX format (line 160), ensuring platform independence. The method correctly letsRuntimeErrorpropagate fromapply_patch(line 162), aligning with the project's preference for explicit failures. Based on learnings, this is the expected behavior.Minor optimization: Avoid duplicate path computation.
Lines 151 and 160 both compute
patch_path.relative_to(cwd). Consider storing the result to avoid redundant computation:♻️ Suggested optimization
def _apply_patches(self) -> list[str]: """Apply the patches.""" cwd = pathlib.Path(".").resolve() applied_patches = [] for patch in self.__project.patch: patch_path = (cwd / patch).resolve() try: - patch_path.relative_to(cwd) + relative_patch = patch_path.relative_to(cwd) except ValueError: self._log_project(f'Skipping patch "{patch}" which is outside {cwd}.') continue if not patch_path.exists(): self._log_project(f"Skipping non-existent patch {patch}") continue - normalized_patch_path = str(patch_path.relative_to(cwd).as_posix()) + normalized_patch_path = str(relative_patch.as_posix()) apply_patch(normalized_patch_path, root=self.local_path) self._log_project(f'Applied patch "{normalized_patch_path}"') applied_patches.append(normalized_patch_path) return applied_patches
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dfetch/project/subproject.py
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: spoorcc
Repo: dfetch-org/dfetch PR: 942
File: dfetch/project/subproject.py:143-166
Timestamp: 2026-01-09T23:16:31.139Z
Learning: In dfetch project's dfetch/project/subproject.py, patch application failures should fail immediately by letting RuntimeError propagate from apply_patch() rather than catching and continuing with remaining patches. The project prefers explicit failures for patch operations.
📚 Learning: 2026-01-09T23:16:31.139Z
Learnt from: spoorcc
Repo: dfetch-org/dfetch PR: 942
File: dfetch/project/subproject.py:143-166
Timestamp: 2026-01-09T23:16:31.139Z
Learning: In dfetch project's dfetch/project/subproject.py, patch application failures should fail immediately by letting RuntimeError propagate from apply_patch() rather than catching and continuing with remaining patches. The project prefers explicit failures for patch operations.
Applied to files:
dfetch/project/subproject.py
📚 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/project/subproject.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/project/subproject.py
📚 Learning: 2026-01-02T22:46:53.305Z
Learnt from: spoorcc
Repo: dfetch-org/dfetch PR: 904
File: script/package.py:10-18
Timestamp: 2026-01-02T22:46:53.305Z
Learning: In dfetch's script/package.py, importing dfetch.__version__ directly is acceptable and intentional; if dfetch is not installed in the environment, the packaging job should fail explicitly rather than handling the import error gracefully.
Applied to files:
dfetch/project/subproject.py
🧬 Code graph analysis (1)
dfetch/project/subproject.py (3)
dfetch/manifest/project.py (2)
vcs(435-437)patch(415-417)dfetch/project/metadata.py (1)
patch(129-131)dfetch/vcs/patch.py (1)
apply_patch(62-80)
⏰ 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 (macos-latest)
- 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.14)
- GitHub Check: run / run (ubuntu-latest, 3.10)
- GitHub Check: run / run (macos-latest, 3.11)
- GitHub Check: run / run (windows-latest, 3.9)
- GitHub Check: run / run (ubuntu-latest, 3.13)
- GitHub Check: run / run (windows-latest, 3.10)
- GitHub Check: run / run (windows-latest, 3.11)
- GitHub Check: run / run (macos-latest, 3.10)
- GitHub Check: run / run (windows-latest, 3.12)
- GitHub Check: run / run (windows-latest, 3.13)
- GitHub Check: test / test
- GitHub Check: run / test-cygwin
- GitHub Check: DevContainer Build & Test
🔇 Additional comments (1)
dfetch/project/subproject.py (1)
19-19: LGTM! Good centralization of patch handling.The import of
apply_patchfrom the newdfetch.vcs.patchmodule supports the refactored patch application flow and provides better separation of concerns.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
dfetch/project/subproject.py (1)
153-157: Minor: Inconsistent quote formatting in log messages.Line 153 quotes the patch variable:
f'Skipping patch "{patch}"...'
Line 157 doesn't:f"Skipping non-existent patch {patch}"Consider using consistent formatting for both messages.
Minor formatting fix
- self._log_project(f"Skipping non-existent patch {patch}") + self._log_project(f'Skipping non-existent patch "{patch}"')
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dfetch/project/subproject.py
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2026-01-09T23:16:31.139Z
Learnt from: spoorcc
Repo: dfetch-org/dfetch PR: 942
File: dfetch/project/subproject.py:143-166
Timestamp: 2026-01-09T23:16:31.139Z
Learning: In dfetch project's dfetch/project/subproject.py, patch application failures should fail immediately by letting RuntimeError propagate from apply_patch() rather than catching and continuing with remaining patches. The project prefers explicit failures for patch operations.
Applied to files:
dfetch/project/subproject.py
📚 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/project/subproject.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/project/subproject.py
📚 Learning: 2026-01-02T22:46:53.305Z
Learnt from: spoorcc
Repo: dfetch-org/dfetch PR: 904
File: script/package.py:10-18
Timestamp: 2026-01-02T22:46:53.305Z
Learning: In dfetch's script/package.py, importing dfetch.__version__ directly is acceptable and intentional; if dfetch is not installed in the environment, the packaging job should fail explicitly rather than handling the import error gracefully.
Applied to files:
dfetch/project/subproject.py
🧬 Code graph analysis (1)
dfetch/project/subproject.py (3)
dfetch/manifest/project.py (2)
vcs(435-437)patch(415-417)dfetch/project/metadata.py (1)
patch(129-131)dfetch/vcs/patch.py (1)
apply_patch(62-80)
⏰ 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 (ubuntu-latest)
- GitHub Check: build / build (macos-latest)
- GitHub Check: build / build (windows-latest)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: run / run (windows-latest, 3.9)
- GitHub Check: run / run (macos-latest, 3.9)
- GitHub Check: run / run (windows-latest, 3.11)
- GitHub Check: run / run (windows-latest, 3.12)
- GitHub Check: run / run (ubuntu-latest, 3.13)
- GitHub Check: run / run (windows-latest, 3.10)
- GitHub Check: run / run (windows-latest, 3.13)
- GitHub Check: run / run (macos-latest, 3.14)
- GitHub Check: run / run (macos-latest, 3.10)
- GitHub Check: run / run (windows-latest, 3.14)
- GitHub Check: DevContainer Build & Test
- GitHub Check: run / test-cygwin
- GitHub Check: test / test
🔇 Additional comments (3)
dfetch/project/subproject.py (3)
19-19: LGTM! Centralized patch application.Importing the centralized
apply_patchfunction improves code reuse and leverages robust error handling (UTF-8 fallback, validation) already implemented indfetch.vcs.patch.
131-131: LGTM! Clean delegation to dedicated patch application method.The refactoring improves separation of concerns and ensures normalized patch paths are properly recorded in metadata.
142-165: Excellent implementation! Addresses cross-platform patch path issue.This method correctly implements the PR objective of storing platform-independent, resolved relative paths for patches. Key improvements:
- Line 160:
.as_posix()normalization ensures Windows paths use forward slashes, addressing issue #937- Lines 150-154: Security check prevents patches outside the project directory
- Lines 156-158: Graceful handling of missing patches
- Line 162: Delegates to centralized
apply_patch()with proper error propagation (per learnings)The "calculate relative patch only once" optimization from the commit message is achieved by computing
relative_patch_pathonce at line 151 and reusing the normalized form.
Fixes #937
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.