Load manifest only once #927
Conversation
WalkthroughConsolidates manifest parsing/validation into a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as CLI (commands / project)
participant Parse as dfetch.manifest.parse
participant FS as File System
participant Logger as Logger
CLI->>Parse: find_manifest()
Parse->>FS: find_file(DEFAULT_MANIFEST_NAME, ".")
FS-->>Parse: paths[]
alt multiple manifests
Parse->>Logger: warn about multiples
end
Parse->>FS: read selected manifest file
FS-->>Parse: raw YAML text
Parse->>Parse: validate YAML against MANIFEST_SCHEMA
alt validation OK
Parse->>Parse: enforce uniqueness (remotes.name, projects.name/dst)
Parse-->>CLI: return Manifest (text & path)
else validation error
Parse-->>CLI: raise RuntimeError (snippet & problem)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
ben-edna
force-pushed
the
validate-and-parse
branch
from
0b9163f to
0531c13
Compare
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/test_update.py (1)
60-62: Inconsistent mock path - test will not mock correctly.The mock path
dfetch.manifest.manifest.get_childmanifestsis stale. Sinceget_childmanifestshas been moved todfetch.manifest.parse, this mock will not intercept the actual function call, causing the test to behave unexpectedly. Update to match the fix applied intest_updateabove.🔎 Proposed fix
with patch("dfetch.commands.update.SuperProject", return_value=fake_superproject): with patch( - "dfetch.manifest.manifest.get_childmanifests" + "dfetch.manifest.parse.get_childmanifests" ) as mocked_get_childmanifests:dfetch/manifest/project.py (1)
340-347: Potential behavioral change whennameis missing from YAML data.Initializing
kwargswith{"name": ""}changes behavior: previously, ifyamldatalacked anamekey, the__init__check at line 309-310 (if "name" not in kwargs) would raiseRuntimeError. Now, the check passes with an empty string, creating aProjectEntrywithself._name = "".If the intent is to satisfy the
Required[str]type annotation, consider keeping stricter runtime validation:🔎 Alternative approach
- kwargs: ProjectEntryDict = {"name": ""} + kwargs: ProjectEntryDict = {"name": yamldata.get("name", "")} # type: ignore[typeddict-item] + if not kwargs["name"]: + raise RuntimeError("Name missing from Project!") for key in ProjectEntryDict.__annotations__.keys(): # pylint: disable=no-member + if key == "name": + continue # Already handled above try: kwargs[str(key)] = yamldata[key] # type: ignore except KeyError: passOr simply validate after the loop:
kwargs: ProjectEntryDict = {"name": ""} for key in ProjectEntryDict.__annotations__.keys(): try: kwargs[str(key)] = yamldata[key] except KeyError: pass if not kwargs.get("name"): raise RuntimeError("Name missing from Project!")
🤖 Fix all issues with AI agents
In @dfetch/commands/update.py:
- Line 36: Remove the unused import dfetch.manifest.parse from update.py; the
file already imports check_child_manifests from dfetch.commands.common which
uses parse internally, so delete the standalone import statement (the line
"import dfetch.manifest.parse") to eliminate dead code and unused imports.
🧹 Nitpick comments (1)
dfetch/manifest/parse.py (1)
64-66: Consider refining types to eliminate# type: ignorecomments.The type ignores are needed because
remotesandprojectsare extracted asAnyfrom the YAML data. If the schema guarantees they're lists of dicts, you could add runtime type narrowing or more specific type hints to improve type safety.🔎 Example approach
# After extracting from YAML, assert the expected types: remotes = manifest.get("remotes", []) or [] projects = manifest["projects"] # Add type assertions if schema guarantees structure if not isinstance(remotes, list): remotes = [] if not isinstance(projects, list): raise RuntimeError("manifest.projects must be a list") _ensure_unique(remotes, "name", "manifest.remotes") _ensure_unique(projects, "name", "manifest.projects") _ensure_unique(projects, "dst", "manifest.projects")This would make the types explicit and remove the need for
# type: ignore.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
dfetch/commands/common.pydfetch/commands/update.pydfetch/commands/validate.pydfetch/manifest/manifest.pydfetch/manifest/parse.pydfetch/manifest/project.pydfetch/manifest/validate.pydfetch/project/superproject.pypyproject.tomlscript/create_sbom.pyscript/package.pytests/test_check.pytests/test_manifest.pytests/test_update.py
💤 Files with no reviewable changes (1)
- dfetch/manifest/validate.py
🧰 Additional context used
🧠 Learnings (2)
📚 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/commands/update.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/superproject.py
🧬 Code graph analysis (5)
dfetch/commands/update.py (1)
dfetch/manifest/parse.py (1)
parse(36-68)
dfetch/commands/validate.py (1)
dfetch/manifest/parse.py (2)
parse(36-68)find_manifest(71-82)
dfetch/commands/common.py (2)
dfetch/manifest/manifest.py (1)
Manifest(103-356)dfetch/manifest/parse.py (2)
parse(36-68)get_childmanifests(85-100)
tests/test_manifest.py (1)
dfetch/manifest/parse.py (3)
parse(36-68)find_manifest(71-82)get_childmanifests(85-100)
dfetch/manifest/manifest.py (3)
dfetch/manifest/project.py (2)
version(363-368)version(371-377)dfetch/project/metadata.py (1)
version(84-86)dfetch/manifest/remote.py (2)
RemoteDict(28-31)Remote(34-98)
⏰ 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). (14)
- 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 (macos-latest, 3.11)
- 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 (windows-latest, 3.11)
- GitHub Check: run / run (windows-latest, 3.9)
- GitHub Check: run / run (macos-latest, 3.14)
- GitHub Check: run / run (windows-latest, 3.10)
- GitHub Check: run / test-cygwin
- GitHub Check: DevContainer Build & Test
🔇 Additional comments (20)
pyproject.toml (1)
192-193: Verify intent behind disabling pyright standard mode.The
standardsetting has been commented out, which disables stricter type checking for thedfetchandfeaturesdirectories. Was this intentional to address specific pyright issues during the manifest refactor, or should it be re-enabled once the refactor is complete?script/create_sbom.py (1)
18-22: LGTM!The inline conditional expression is functionally equivalent to the previous if/elif/else chain and improves conciseness. The platform mapping logic (darwin → osx, win → win, else → nix) is preserved correctly.
script/package.py (1)
49-53: LGTM!Consistent with the refactor in
script/create_sbom.py. The inline conditional maintains the same platform detection logic.dfetch/commands/common.py (1)
8-9: LGTM!The import paths are correctly updated to reflect the new module structure.
Manifestremains indfetch.manifest.manifestwhileget_childmanifestsis now imported from the newdfetch.manifest.parsemodule, consistent with the PR's refactoring goal.dfetch/manifest/project.py (1)
277-277: Good use ofRequiredfor explicit typing.Using
Required[str]for thenamefield in atotal=FalseTypedDict correctly documents thatnameis mandatory while other fields remain optional. This improves type safety and IDE support.Also applies to: 285-285
dfetch/commands/validate.py (2)
14-14: LGTM! Clean import consolidation.The import path correctly reflects the relocation of
find_manifestandparseto the newdfetch.manifest.parsemodule.
36-36: LGTM! Validation now handled by parse.The replacement of
validate()withparse()is correct, asparse()performs validation as part of the parsing process.tests/test_check.py (1)
37-37: LGTM! Mock path correctly updated.The mock target properly reflects the relocation of
get_childmanifeststodfetch.manifest.parse.tests/test_manifest.py (3)
18-18: LGTM! Import path correctly updated.The import statement properly reflects the relocation of
find_manifestandget_childmanifeststo the parse module.
101-114: LGTM! Mock paths consistently updated.All mock paths for
find_filecorrectly reference the new module locationdfetch.manifest.parse.
139-154: LGTM! Test correctly adapted for parse module.The test properly mocks
dfetch.manifest.parse.parseand verifies thatparseis called with the correct manifest paths. The assertion logic usingparse_mock.call_args_listis correct.dfetch/project/superproject.py (2)
16-17: LGTM! Clean separation of imports.The imports correctly reflect the reorganization:
Manifestclass remains indfetch.manifest.manifest, while parsing utilities are now imported fromdfetch.manifest.parse.
42-42: LGTM! Manifest loading consolidated.The change from
Manifest.from_file()toparse()correctly uses the new parsing function, which validates and returns aManifestobject. This aligns with the PR objective of consolidating manifest loading.dfetch/manifest/manifest.py (1)
30-30: LGTM! Improved TypedDict typing.The typing refinement is well done:
total=Truemakes the TypedDict more explicit about required fieldsNotRequiredproperly marksremotesas optional while keepingversionandprojectsrequired- This improves type safety and clarity
Also applies to: 95-100
dfetch/manifest/parse.py (6)
1-16: LGTM!The imports are well-organized and appropriate for the manifest validation and parsing functionality.
18-33: LGTM!The uniqueness validation logic is correct and the error messages are clear and actionable.
36-68: Validation logic is solid.The manifest parsing, error handling, and uniqueness validation are well-implemented. The error messages with YAML snippets will be helpful for debugging.
71-82: LGTM!The manifest discovery logic correctly handles edge cases (no manifests, multiple manifests) with appropriate errors and warnings.
85-99: Function structure is well-designed.The child manifest discovery logic is clear, with good logging and error context wrapping. The skip list handling is appropriate for avoiding duplicate processing.
95-97: Verify the path computation for error context.The relative path computation uses
os.path.dirname(os.getcwd()), making paths relative to the parent of the current working directory rather than the current directory itself. While this works in practice (sincefind_file()starts from the current directory, paths will be under its parent), this pattern is unusual and the intent is unclear.Consider whether this should be
os.getcwd()instead, or if this is intentional for a specific directory structure. The test doesn't validate the actual path format, so adding a comment explaining the design choice would help.
ben-edna
force-pushed
the
validate-and-parse
branch
from
0531c13 to
9000605
Compare
ben-edna
force-pushed
the
validate-and-parse
branch
from
9000605 to
c0e5766
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @dfetch/commands/update.py:
- Line 36: Remove the unused top-level import "import dfetch.manifest.parse"
from the module; the file already imports and uses check_child_manifests (from
dfetch.commands.common) which calls parse internally, so delete the standalone
import statement to eliminate the unused dependency and keep imports minimal.
In @dfetch/manifest/parse.py:
- Around line 85-86: Update the docstring for get_childmanifests(skip:
Optional[list[str]] = None) -> list[Manifest] to accurately describe that the
function returns a list of child Manifest objects (optionally excluding names in
the skip parameter), rather than "Get manifest and its path"; include brief
mention of the return type and purpose of the skip argument so readers
understand it yields multiple child manifests.
- Around line 95-97: The call to
pathlib.Path(path).relative_to(os.path.dirname(os.getcwd())).as_posix() can
raise ValueError when path is not a subpath of os.path.dirname(os.getcwd()), and
that exception occurs outside the prefix_runtime_exceptions context; wrap that
relative path computation in a try/except ValueError and fall back to a safe
representation (e.g., pathlib.Path(path).as_posix() or the resolved absolute
path) before entering prefix_runtime_exceptions so prefix_runtime_exceptions
always sees a valid string; update the code around prefix_runtime_exceptions to
compute display_path using a try/except around relative_to and then pass
display_path into prefix_runtime_exceptions.
- Around line 45-56: The code imports and catches StrictYAMLError which doesn't
exist in strictyaml 1.7.3; remove StrictYAMLError from the import and from the
except clause and either catch only YAMLValidationError or replace it with
YAMLError if you want the broader base class. Update the import line to import
YAMLValidationError (and YAMLError if chosen) instead of StrictYAMLError, and
change the exception handler in the parse logic (the except that currently
references StrictYAMLError) to either "except YAMLValidationError as err" or
"except (YAMLValidationError, YAMLError) as err" so the module no longer raises
ImportError at import time while preserving the existing usage of
err.context_mark.get_snippet() and err.problem.
🧹 Nitpick comments (4)
script/package.py (1)
49-53: Consider a more readable approach for platform detection.The nested ternary expression is functionally correct but can be harder to read and maintain. A dictionary mapping would be clearer and more extensible.
🔎 Proposed refactor using a dictionary
-PLATFORM_NAME = ( - "osx" - if sys.platform.startswith("darwin") - else "win" if sys.platform.startswith("win") else "nix" -) +PLATFORM_MAPPING = { + "darwin": "osx", + "win": "win", +} +PLATFORM_NAME = next( + (name for prefix, name in PLATFORM_MAPPING.items() if sys.platform.startswith(prefix)), + "nix" +)script/create_sbom.py (1)
18-22: Consider extracting platform detection to reduce duplication.This nested ternary pattern is duplicated in
script/package.py(lines 49-53). Consider extracting platform detection to a shared utility function or constant to maintain DRY principles.💡 Suggested approach
Create a shared function or move to a common module:
def get_platform_name() -> str: """Get the platform name for packaging.""" if sys.platform.startswith("darwin"): return "osx" elif sys.platform.startswith("win"): return "win" else: return "nix"dfetch/manifest/parse.py (2)
64-66: Consider removingtype: ignorecomments.The
# type: ignorecomments suppress type checking and may hide legitimate type mismatches. Ifremotesandprojectsare guaranteed to belist[dict[str, Any]]by the schema, consider adding explicit type assertions or narrowing the types in_ensure_uniqueto accept the actual types from the schema.
62-62: Simplify redundant default handling.The expression
manifest.get("remotes", []) or []contains redundant logic. Theor []guard is only needed if the manifest might explicitly storeNoneas the value. If the schema prevents this, simplify tomanifest.get("remotes", []).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
dfetch/commands/common.pydfetch/commands/update.pydfetch/commands/validate.pydfetch/manifest/manifest.pydfetch/manifest/parse.pydfetch/manifest/project.pydfetch/manifest/validate.pydfetch/project/superproject.pypyproject.tomlscript/create_sbom.pyscript/package.pytests/test_check.pytests/test_manifest.pytests/test_update.py
💤 Files with no reviewable changes (1)
- dfetch/manifest/validate.py
🚧 Files skipped from review as they are similar to previous changes (7)
- tests/test_check.py
- tests/test_update.py
- pyproject.toml
- dfetch/manifest/project.py
- dfetch/project/superproject.py
- dfetch/commands/validate.py
- dfetch/commands/common.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/commands/update.py
🧬 Code graph analysis (1)
dfetch/commands/update.py (1)
dfetch/manifest/parse.py (1)
parse(36-68)
⏰ 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 (macos-latest)
- GitHub Check: build / build (windows-latest)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: run / run (macos-latest, 3.11)
- GitHub Check: run / run (windows-latest, 3.13)
- GitHub Check: run / run (windows-latest, 3.10)
- 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 (windows-latest, 3.14)
- GitHub Check: run / run (windows-latest, 3.9)
- GitHub Check: run / run (ubuntu-latest, 3.9)
- GitHub Check: run / test-cygwin
- GitHub Check: DevContainer Build & Test
🔇 Additional comments (6)
dfetch/manifest/manifest.py (1)
95-100: LGTM! TypedDict improvements enhance type safety.The change from
total=Falsetototal=TruewithNotRequiredfor optional fields is a best practice that makes the schema more explicit. This correctly reflects thatremotesis optional (as confirmed by theget("remotes", [])usage at line 126) while other fields likeversionandprojectsare required.tests/test_manifest.py (3)
18-18: LGTM! Import updated to reflect module relocation.The import correctly references the relocated functions
find_manifestandget_childmanifestsfrom the newdfetch.manifest.parsemodule.
101-119: LGTM! Test patches correctly updated.The patches for
find_filehave been correctly updated to targetdfetch.manifest.parse.find_fileinstead of the old module path. The test logic remains intact.
139-155: LGTM! Test refactored to match new parse module.The test correctly:
- Patches
dfetch.manifest.parse.find_fileanddfetch.manifest.parse.parse- Uses
parse_mock.call_args_listto verify parse calls- Maintains the same test coverage with updated module paths
dfetch/manifest/parse.py (2)
18-33: LGTM!The uniqueness check logic is correct and provides clear error messages when duplicates are detected.
71-82: LGTM!The function correctly handles the cases of zero, one, and multiple manifests with appropriate error handling and logging.
dfetch/manifest/parse.py
Outdated
| def get_childmanifests(skip: Optional[list[str]] = None) -> list[Manifest]: | ||
| """Get manifest and its path.""" |
There was a problem hiding this comment.
Update docstring to accurately describe the function.
The docstring "Get manifest and its path." is misleading. The function returns a list of child manifests, not a single manifest or its path.
🔎 Proposed fix
def get_childmanifests(skip: Optional[list[str]] = None) -> list[Manifest]:
- """Get manifest and its path."""
+ """Find and parse all child manifest files, excluding paths in skip list."""
skip = skip or []📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def get_childmanifests(skip: Optional[list[str]] = None) -> list[Manifest]: | |
| """Get manifest and its path.""" | |
| def get_childmanifests(skip: Optional[list[str]] = None) -> list[Manifest]: | |
| """Find and parse all child manifest files, excluding paths in skip list.""" | |
| skip = skip or [] |
🤖 Prompt for AI Agents
In @dfetch/manifest/parse.py around lines 85 - 86, Update the docstring for
get_childmanifests(skip: Optional[list[str]] = None) -> list[Manifest] to
accurately describe that the function returns a list of child Manifest objects
(optionally excluding names in the skip parameter), rather than "Get manifest
and its path"; include brief mention of the return type and purpose of the skip
argument so readers understand it yields multiple child manifests.
ben-edna
force-pushed
the
validate-and-parse
branch
from
c0e5766 to
17f0225
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
script/create_sbom.py (1)
18-29: LGTM! Clean refactoring of platform detection logic.The data-driven approach using
PLATFORM_MAPPINGandnext()is more maintainable than if/elif chains. The logic correctly handles common platform values (darwin→osx, win32→win, linux→nix).Note: This logic is duplicated in
script/package.py(lines 49-60). Consider extracting it to a shared utility if these scripts evolve further, though duplication is acceptable for standalone scripts.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
dfetch/commands/common.pydfetch/commands/update.pydfetch/commands/validate.pydfetch/manifest/manifest.pydfetch/manifest/parse.pydfetch/manifest/project.pydfetch/manifest/validate.pydfetch/project/superproject.pypyproject.tomlscript/create_sbom.pyscript/package.pytests/test_check.pytests/test_manifest.pytests/test_update.py
💤 Files with no reviewable changes (1)
- dfetch/manifest/validate.py
🚧 Files skipped from review as they are similar to previous changes (9)
- dfetch/commands/common.py
- pyproject.toml
- dfetch/manifest/project.py
- dfetch/manifest/parse.py
- tests/test_manifest.py
- tests/test_update.py
- dfetch/project/superproject.py
- tests/test_check.py
- dfetch/commands/validate.py
🧰 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/commands/update.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/commands/update.py
🧬 Code graph analysis (1)
dfetch/manifest/manifest.py (1)
dfetch/manifest/remote.py (2)
RemoteDict(28-31)Remote(34-98)
⏰ 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 (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 (windows-latest, 3.13)
- GitHub Check: run / run (windows-latest, 3.11)
- GitHub Check: run / run (windows-latest, 3.10)
- GitHub Check: run / run (macos-latest, 3.10)
- GitHub Check: run / run (windows-latest, 3.12)
- GitHub Check: run / run (windows-latest, 3.9)
- GitHub Check: run / run (ubuntu-latest, 3.11)
- GitHub Check: run / test-cygwin
- GitHub Check: test / test
- GitHub Check: DevContainer Build & Test
🔇 Additional comments (4)
dfetch/commands/update.py (1)
36-36: LGTM! Clean import simplification.The refactoring to import the parent
dfetch.projectmodule instead of specific submodules (likedfetch.project.gitanddfetch.project.svn) is cleaner and appropriate for the usage at line 88 wheredfetch.project.make()is called.dfetch/manifest/manifest.py (2)
30-30: LGTM! Correct import for NotRequired.The addition of
NotRequiredfromtyping_extensionsis the proper way to mark optional fields in a TypedDict withtotal=True.
95-100: Improved TypedDict pattern with explicit required/optional fields.The refactoring from
total=Falsetototal=TruewithNotRequiredfor optional fields is a cleaner pattern that makes the required fields (version,projects) more explicit.One minor observation: Line 118 uses
manifest.get("version", self.CURRENT_VERSION), treatingversionas potentially missing despite it being marked as required in the TypedDict. This is likely intentional defensive programming or backwards compatibility support, which is acceptable.script/package.py (1)
49-60: LGTM! Consistent platform detection refactoring.The implementation matches the approach in
script/create_sbom.pyand improves maintainability. The platform mapping correctly handles target platforms (darwin → osx, win → win, others → nix).
Summary by CodeRabbit
Refactor
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.