Make all paths absolute and resolve them (fix #946) #947
Conversation
WalkthroughSuperProject now stores its root as a resolved absolute pathlib.Path, validates and resolves paths via a new util, and delegates ignored-files resolution to VCS-specific implementations (Git or SVN). in_directory accepts Path inputs and resolve_absolute_path was added. Changes
Sequence Diagram(s)sequenceDiagram
actor Caller
participant SuperProject
participant Util as resolve_absolute_path
participant VCS as VCS Repo (Git/SVN)
Caller->>SuperProject: ignored_files(path)
SuperProject->>Util: resolve_absolute_path(path)
Util-->>SuperProject: resolved_path (Path)
SuperProject->>SuperProject: check resolved_path.is_relative_to(root_directory)
alt path outside root
SuperProject-->>Caller: raise RuntimeError
else path inside root
SuperProject->>VCS: delegate ignored_files(resolved_path)
VCS-->>SuperProject: list_of_ignored_files
SuperProject-->>Caller: list_of_ignored_files
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 1
🤖 Fix all issues with AI agents
In @dfetch/project/superproject.py:
- Around line 68-73: The code compares
os.path.commonprefix((pathlib.Path(path).absolute().resolve(),
self.root_directory)) to self.root_directory using a Path object against a str;
convert the Path to a string before calling commonprefix to ensure consistent
types. Replace the first tuple element (the result of
pathlib.Path(path).absolute().resolve()) with
str(pathlib.Path(path).absolute().resolve()) so os.path.commonprefix is
comparing two strings, and keep the comparison target as self.root_directory.
🧹 Nitpick comments (2)
dfetch/project/superproject.py (2)
43-45: Minor:absolute()is redundant beforeresolve().
Path.resolve()already returns an absolute path (and also resolves symlinks), so callingabsolute()first is unnecessary. This applies here and at line 70 and 76.Suggested simplification
self._root_directory = str( - pathlib.Path(os.path.dirname(self._manifest.path)).absolute().resolve() + pathlib.Path(os.path.dirname(self._manifest.path)).resolve() )Similarly for line 70:
- (pathlib.Path(path).absolute().resolve(), self.root_directory) + (pathlib.Path(path).resolve(), self.root_directory)And line 76:
- f"It resolves to {pathlib.Path(path).resolve()}" + f"It resolves to {pathlib.Path(path).resolve()}"(Line 76 already uses just
resolve(), which is correct.)
68-73: Pre-existing note:commonprefixis character-based, not path-aware.This is not introduced by this PR, but worth noting:
os.path.commonprefixperforms character-by-character comparison, not path-segment comparison. For example, paths/projectand/projectXwould have a common prefix of/project, potentially causing false positives.A more robust approach would use
Path.is_relative_to()(Python 3.9+) or compare path parts:resolved_path = pathlib.Path(path).resolve() root_path = pathlib.Path(self.root_directory) if not resolved_path.is_relative_to(root_path): raise RuntimeError(...)This is optional and can be addressed separately if needed.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dfetch/project/superproject.py
🧰 Additional context used
🧠 Learnings (2)
📓 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.
📚 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/superproject.py
⏰ 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). (20)
- 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: build / Build wheel 📦
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: run / run (windows-latest, 3.14)
- GitHub Check: run / run (macos-latest, 3.10)
- GitHub Check: run / run (macos-latest, 3.9)
- GitHub Check: run / run (ubuntu-latest, 3.13)
- GitHub Check: run / run (windows-latest, 3.9)
- GitHub Check: run / run (windows-latest, 3.13)
- GitHub Check: run / run (windows-latest, 3.10)
- GitHub Check: run / run (windows-latest, 3.12)
- GitHub Check: run / run (macos-latest, 3.12)
- GitHub Check: run / run (windows-latest, 3.11)
- GitHub Check: DevContainer Build & Test
- GitHub Check: test / test
- GitHub Check: docs / Documentation
- GitHub Check: run / test-cygwin
🔇 Additional comments (1)
dfetch/project/superproject.py (1)
74-77: Good improvement to error diagnostics.Including both the original path and the resolved path in the error message will help users understand why their path is being rejected, especially when relative paths or symlinks are involved.
spoorcc
force-pushed
the
ben-edna/issue946
branch
from
776d16f to
3e0cf67
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
dfetch/project/superproject.py (2)
77-80: Consider passing the resolved path to VCS methods for consistency.The validation at line 72 uses
resolved_path, but the VCS calls on lines 78 and 80 still pass the originalpathparameter. If the original path is relative or contains symlinks, the VCS methods might resolve it differently, leading to inconsistent behavior.♻️ Proposed fix
if GitLocalRepo(self.root_directory).is_git(): - return GitLocalRepo.ignored_files(path) + return GitLocalRepo.ignored_files(str(resolved_path)) if SvnRepo(self.root_directory).is_svn(): - return SvnRepo.ignored_files(path) + return SvnRepo.ignored_files(str(resolved_path))
73-75: Nit: trailing space in error message.There's a trailing space before the closing quote on line 74-75.
♻️ Suggested fix
if not resolved_path.is_relative_to(root_path): raise RuntimeError( - f"{resolved_path} not in superproject {self.root_directory}! " + f"{resolved_path} not in superproject {self.root_directory}!" )
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dfetch/project/superproject.pydfetch/util/util.py
🧰 Additional context used
🧠 Learnings (2)
📓 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.
📚 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/superproject.py
🧬 Code graph analysis (1)
dfetch/util/util.py (2)
dfetch/project/metadata.py (1)
path(134-142)dfetch/manifest/manifest.py (1)
path(256-258)
⏰ 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 wheel 📦
- GitHub Check: build / build (macos-latest)
- GitHub Check: build / build (windows-latest)
- GitHub Check: run / run (windows-latest, 3.12)
- GitHub Check: DevContainer Build & Test
- GitHub Check: run / run (macos-latest, 3.10)
- GitHub Check: run / run (windows-latest, 3.13)
- GitHub Check: run / run (windows-latest, 3.9)
- 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.10)
- GitHub Check: run / run (macos-latest, 3.12)
- GitHub Check: run / run (macos-latest, 3.13)
- GitHub Check: run / test-cygwin
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (2)
dfetch/util/util.py (1)
164-172: LGTM!The utility function correctly handles path normalization by chaining
expanduser()beforerealpath(), ensuring tilde expansion occurs before symlink resolution. This approach is consistent with the existing pattern indfetch/project/metadata.pyand properly addresses cross-platform path handling.dfetch/project/superproject.py (1)
44-46: LGTM!Storing the root directory as an absolute resolved path ensures consistent path comparisons throughout the class, directly addressing issue #946.
spoorcc
force-pushed
the
ben-edna/issue946
branch
from
3e0cf67 to
14db820
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @dfetch/util/util.py:
- Around line 164-174: The docstring for resolve_absolute_path incorrectly
documents the parameter as "data: A list of strings"; update the Args section to
describe the actual parameter: "path: Union[str, Path]" and a short description
like "Path or string to convert to an absolute, symlink-resolved Path" (keep
notes about expanding '~' and Windows handling intact); ensure the docstring
parameter name matches the function signature and remove the erroneous "data"
entry so the docs accurately reflect resolve_absolute_path.
🧹 Nitpick comments (2)
dfetch/project/superproject.py (2)
67-80: Consider usingresolved_pathconsistently in VCS calls.The validation at lines 72-75 uses
resolved_path, but the VCS calls at lines 78 and 80 still use the originalpath. If the input was relative or contained symlinks, this could lead to inconsistent behavior. Based on the learning, ignored files should be queried from the project's destination path using the superproject's VCS system—using the resolved path would ensure consistency.♻️ Suggested change
if GitLocalRepo(self.root_directory).is_git(): - return GitLocalRepo.ignored_files(path) + return GitLocalRepo.ignored_files(str(resolved_path)) if SvnRepo(self.root_directory).is_svn(): - return SvnRepo.ignored_files(path) + return SvnRepo.ignored_files(str(resolved_path))
73-75: Minor: trailing space in error message.The error message string ends with a trailing space after the
!.♻️ Suggested fix
raise RuntimeError( - f"{resolved_path} not in superproject {self.root_directory}! " + f"{resolved_path} not in superproject {self.root_directory}!" )
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dfetch/project/superproject.pydfetch/util/util.py
🧰 Additional context used
🧠 Learnings (2)
📓 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.
📚 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/superproject.py
🧬 Code graph analysis (2)
dfetch/util/util.py (2)
dfetch/project/metadata.py (1)
path(134-142)dfetch/manifest/manifest.py (1)
path(256-258)
dfetch/project/superproject.py (2)
dfetch/util/util.py (1)
resolve_absolute_path(164-174)dfetch/manifest/manifest.py (1)
path(256-258)
⏰ 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.12)
- GitHub Check: run / run (windows-latest, 3.10)
- GitHub Check: run / run (ubuntu-latest, 3.9)
- GitHub Check: run / run (windows-latest, 3.9)
- GitHub Check: run / run (windows-latest, 3.14)
- GitHub Check: run / run (macos-latest, 3.10)
- GitHub Check: run / run (windows-latest, 3.11)
- GitHub Check: run / run (windows-latest, 3.13)
- GitHub Check: test / test
- GitHub Check: run / test-cygwin
- GitHub Check: DevContainer Build & Test
🔇 Additional comments (2)
dfetch/project/superproject.py (2)
44-46: LGTM!Using
resolve_absolute_pathto normalize the root directory addresses issue #946 by ensuring consistent absolute path comparison for child folder detection.
79-80: LGTM!Adding SVN support for
ignored_filescorrectly mirrors the existing Git handling and aligns with the pattern of using the superproject's VCS type. Based on learnings, this is the correct approach.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @dfetch/util/util.py:
- Around line 164-174: The docstring for resolve_absolute_path wrongly documents
`data`; update the Args section to document the actual parameter `path` (e.g.,
"path: Union[str, Path] — the file system path or path-like object to be
expanded and converted to an absolute Path"), and ensure the description
mentions that it expands '~' and resolves symlinks; keep the existing Notes
intact and align the type/signature in the docstring with the function
signature.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
dfetch/project/superproject.pydfetch/util/util.pydfetch/vcs/git.pydfetch/vcs/svn.pytests/test_check.pytests/test_report.pytests/test_update.py
🧰 Additional context used
🧠 Learnings (2)
📓 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.
📚 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:
tests/test_check.pytests/test_update.pytests/test_report.pydfetch/project/superproject.py
🧬 Code graph analysis (5)
tests/test_check.py (1)
dfetch/project/superproject.py (1)
root_directory(49-51)
tests/test_update.py (1)
dfetch/project/superproject.py (1)
root_directory(49-51)
tests/test_report.py (1)
dfetch/project/superproject.py (1)
root_directory(49-51)
dfetch/vcs/git.py (2)
dfetch/project/metadata.py (1)
path(134-142)dfetch/manifest/manifest.py (1)
path(256-258)
dfetch/project/superproject.py (2)
dfetch/util/util.py (1)
resolve_absolute_path(164-174)dfetch/manifest/manifest.py (1)
path(256-258)
⏰ 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 (macos-latest, 3.9)
- GitHub Check: run / run (windows-latest, 3.12)
- GitHub Check: run / run (macos-latest, 3.12)
- GitHub Check: run / run (windows-latest, 3.11)
- GitHub Check: run / run (windows-latest, 3.14)
- GitHub Check: run / run (windows-latest, 3.9)
- GitHub Check: run / run (windows-latest, 3.13)
- GitHub Check: run / run (windows-latest, 3.10)
- GitHub Check: run / run (macos-latest, 3.10)
- GitHub Check: run / run (macos-latest, 3.11)
- GitHub Check: run / test-cygwin
- GitHub Check: test / test
- GitHub Check: DevContainer Build & Test
🔇 Additional comments (9)
dfetch/vcs/git.py (1)
237-239: LGTM!The signature change to accept
Union[str, Path]with internal string normalization is a clean approach that maintains backward compatibility while enabling callers to passPathobjects directly.dfetch/util/util.py (1)
66-75: LGTM!The updated signature accepting
Union[str, Path]with string yielding maintains backward compatibility while enablingPathinputs. The directory check on line 69 works correctly with both types.tests/test_check.py (1)
7-7: LGTM!The mock now correctly uses
Path("/tmp")to match the updatedSuperProject.root_directoryreturn type, ensuring type consistency between tests and production code.Also applies to: 34-34
tests/test_report.py (1)
7-7: LGTM!Consistent with the other test file updates, the mock correctly uses
Path("/tmp")to match the production interface.Also applies to: 34-34
tests/test_update.py (1)
7-7: LGTM!Both
test_updateandtest_forced_updatecorrectly updated to usePath("/tmp")for the mock'sroot_directory, maintaining consistency with theSuperProjectinterface changes.Also applies to: 34-34, 57-57
dfetch/vcs/svn.py (1)
77-82: LGTM!The constructor now accepts both
strandpathlib.Pathinputs while normalizing to string internally. This maintains backward compatibility with existing code that passes strings, while enabling the newPath-based usage fromSuperProject.dfetch/project/superproject.py (3)
44-46: Core fix for issue #946 — paths now consistently resolved.Using
resolve_absolute_pathensures_root_directoryis always an absolute, symlink-resolved path. This is essential for theis_relative_tocheck inignored_filesto work correctly across all scenarios.
67-79: Validation logic and SVN support look correct.The path validation properly resolves both paths to absolute before comparison with
is_relative_to, which fixes the child-folder detection issue from #946. The SVN delegation follows the established pattern and aligns with the learning that ignored files should use the superproject's VCS type. Based on learnings, this correctly uses the superproject's VCS type rather than the fetched project's VCS type.
48-51: No breaking change here — all callers are compatible withpathlib.Path.The return type is
pathlib.Path, but all usages already handle this correctly:in_directory()acceptsUnion[str, Path],GitLocalRepoandSvnRepoconstructors acceptUnion[str, Path], andis_relative_to()is a standard Path method. No code depends on a string return type.Likely an incorrect or invalid review comment.
spoorcc
force-pushed
the
ben-edna/issue946
branch
from
ef17e2e to
c288e36
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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/project/superproject.py (1)
67-79: Fix path inconsistency: useresolved_pathinstead ofpathin VCS method calls.The method computes and validates
resolved_path(which resolves symlinks and ensures absolute paths), but then passes the original unresolvedpathstring toGitLocalRepo.ignored_files()andSvnRepo.ignored_files(). This inconsistency means relative paths or symlinks validated againstresolved_pathbehave differently when passed to the VCS layer.🐛 Proposed fix
if GitLocalRepo(self.root_directory).is_git(): - return GitLocalRepo.ignored_files(path) + return GitLocalRepo.ignored_files(str(resolved_path)) if SvnRepo(self.root_directory).is_svn(): - return SvnRepo.ignored_files(path) + return SvnRepo.ignored_files(str(resolved_path))
🧹 Nitpick comments (1)
dfetch/vcs/svn.py (1)
77-82: LGTM!Consistent with the
GitLocalRepochanges, maintaining backward compatibility while supporting Path objects.Minor style note: This file uses fully-qualified
pathlib.Pathin the type hint (line 79) whilegit.pyimports and uses justPath. Consider aligning for consistency:Optional: Import Path for consistency with git.py
-from typing import NamedTuple, Optional, Union +from pathlib import Path +from typing import NamedTuple, Optional, Union- path: Union[str, pathlib.Path] = ".", + path: Union[str, Path] = ".",
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
dfetch/project/superproject.pydfetch/util/util.pydfetch/vcs/git.pydfetch/vcs/svn.pytests/test_check.pytests/test_report.pytests/test_update.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_check.py
🧰 Additional context used
🧠 Learnings (2)
📓 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.
📚 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:
tests/test_update.pytests/test_report.pydfetch/project/superproject.py
🧬 Code graph analysis (4)
dfetch/util/util.py (2)
dfetch/project/metadata.py (1)
path(134-142)dfetch/manifest/manifest.py (1)
path(256-258)
tests/test_update.py (1)
dfetch/project/superproject.py (1)
root_directory(49-51)
tests/test_report.py (1)
dfetch/project/superproject.py (1)
root_directory(49-51)
dfetch/project/superproject.py (2)
dfetch/util/util.py (1)
resolve_absolute_path(164-174)dfetch/manifest/manifest.py (1)
path(256-258)
⏰ 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). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (8)
dfetch/vcs/git.py (1)
237-239: LGTM!The signature change to accept
Union[str, Path]with internal string normalization is a clean approach that maintains backward compatibility while supporting the new Path-based API used elsewhere in the PR.dfetch/util/util.py (2)
66-75: LGTM!The signature change to accept
Union[str, Path]is appropriate, and yieldingstr(path)ensures consistent return type for callers.
164-174: LGTM!Good choice using
os.path.realpath()overPath.resolve()for better cross-platform reliability, particularly for Windows drive-relative paths. The order of operations (expanduser first, then realpath) is correct.tests/test_update.py (1)
7-7: LGTM!Test mocks correctly updated to use
Path("/tmp")to match the newSuperProject.root_directoryreturn type.Also applies to: 34-34, 57-57
tests/test_report.py (1)
7-7: LGTM!Consistent with the Path-based API changes across the test suite.
Also applies to: 34-34
dfetch/project/superproject.py (3)
22-22: LGTM!The import of
resolve_absolute_pathis appropriate for the path normalization changes in this file.
44-46: LGTM!The root directory is now correctly resolved to an absolute
Path, which addresses the child-folder detection issue from #946 by ensuring consistent path comparison.
48-51: LGTM!The return type change to
pathlib.Pathis consistent with the internal storage, and per the AI summary, downstream callers (GitLocalRepo,SvnRepo) have been updated to acceptUnion[str, Path].
fixes #946
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.