Conversation
Contributor
WalkthroughReplaced Manifest.update_dump() usages with Manifest.dump(); refactored Manifest to derive projects/remotes from the live ruamel YAML document; introduced ManifestBuilder for manifest construction/formatting; added ProjectEntry.from_raw; adjusted manifest dumping API, string normalization, and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "CLI (add/freeze/import)"
participant Manifest as "Manifest / ManifestBuilder"
participant FS as "Filesystem"
CLI->>Manifest: append_project_entry(...) / build()
Note right of Manifest: mutate ruamel YAML doc or assemble YAML string
Manifest->>Manifest: normalize string scalars & adjust spacing
Manifest->>FS: dump(path) — write YAML to file
FS-->>Manifest: write OK
Manifest-->>CLI: return success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dfetch/manifest/manifest.py`:
- Around line 343-358: The projects property rebuilds the project list on every
access causing duplicate work because selected_projects calls self.projects and
then _check_all_names_found calls it again; modify selected_projects to reuse
the already-computed list by passing the local all_projects into the checker (or
update _check_all_names_found to accept an optional projects sequence) instead
of letting _check_all_names_found call self.projects again; touch the
selected_projects method and the _check_all_names_found signature/usage to
accept and use the passed projects to avoid double rebuilds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6fd6b95c-2739-46c7-8506-049b4a093de1
📒 Files selected for processing (6)
dfetch/commands/add.pydfetch/commands/freeze.pydfetch/commands/import_.pydfetch/manifest/manifest.pytests/test_add.pytests/test_manifest.py
spoorcc
force-pushed
the
simplify-manifest-class
branch
from
5b9b3a2 to
6e29cdb
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dfetch/manifest/manifest.py`:
- Around line 262-266: Re-run the spacing and quoting normalizers right before
any serialization so mutations (from append_project_entry and
update_project_version) can't bypass them: call self._ensure_section_spacing()
and self._normalize_string_scalars() at the start of dump() (and similarly
before the immediate serialization path around the other serialize call at the
425-435 area), and/or invoke those two helpers at the end of
append_project_entry() and update_project_version() so the document is
normalized before being written.
- Around line 250-252: The current check calls _ensure_unique(projects_raw,
"dst", "manifest.projects") which only inspects explicit dst keys and misses
collisions where ProjectEntry.__init__ defaults dst to name; update the
validation to compute each project's effective destination (use dst if present
else name) from projects_raw and pass that list or a synthetic field to
_ensure_unique (or call _ensure_unique with the computed values) so duplicates
like a project with no dst and another with dst equal to that name are detected
and rejected.
- Around line 86-92: When clearing potential spurious blank tokens from
parent.ca.items for a given key (the code using parent.ca.items.get(key) and
touching indices [2] and [3]), first inspect the token objects at those
positions and only set them to None if they contain purely whitespace/newlines
(no '#' character) to avoid deleting user comments; similarly update the
analogous logic around the later block that touches indices at lines ~112-115.
Locate the CommentToken (or whatever token object is present) at parent_items[2]
and parent_items[3], examine their string content for a '#' (or non-whitespace)
and preserve the token if it contains a user comment, otherwise clear it as
before. Ensure both places in manifest.py use this same guarded check so user
comments between keys (e.g., between integrity/hash or patch/first item) are
retained.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e0f8cd0e-039f-4abd-a803-a261d8f6b215
📒 Files selected for processing (7)
dfetch/commands/add.pydfetch/commands/freeze.pydfetch/commands/import_.pydfetch/manifest/manifest.pydfetch/manifest/project.pytests/test_add.pytests/test_manifest.py
Contributor
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 (1)
tests/test_manifest.py (1)
420-425: 🧹 Nitpick | 🔵 TrivialTest
dump(), not_doc.as_yaml().The renamed test still bypasses the public API, so it will not catch regressions in
Manifest.dump()itself. Usingmock_open()keeps the assertion aligned with the behavior that changed in this PR.♻️ Suggested change
def test_dump_writes_version_as_quoted_string(manifest_text: str) -> None: """dump must always write version as a quoted YAML string.""" manifest = Manifest.from_yaml(manifest_text) - result = manifest._doc.as_yaml() + with patch("dfetch.manifest.manifest.open", mock_open()) as mocked_open: + manifest.dump(DEFAULT_MANIFEST_NAME) + result = "".join(call.args[0] for call in mocked_open().write.call_args_list) # The version value must appear quoted so that YAML parsers read it as a string. assert f"version: '{manifest.version}'" in result🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_manifest.py` around lines 420 - 425, The test currently calls manifest._doc.as_yaml() instead of the public API; change test_dump_writes_version_as_quoted_string to call Manifest.dump() and capture its output using mock_open (or a tmp file fixture) so you assert against what dump actually writes; locate the test function test_dump_writes_version_as_quoted_string and replace the _doc.as_yaml() usage with a call to manifest.dump() while using mock_open()/patch on builtins.open (or the test's tmp file writer) to read the written content and assert that "version: '<manifest.version>'" appears in that captured output.
♻️ Duplicate comments (2)
dfetch/manifest/manifest.py (2)
354-364:⚠️ Potential issue | 🟠 MajorNormalize the live document immediately before
dump().
_normalize_string_scalars()only runs during construction. After later mutations likeappend_project_entry(), new YAML values are plainstrs again, sodump()can write"176"/"%feature"back unquoted and change how they parse on the next load.♻️ Proposed fix
def dump(self, path: str | None = None) -> None: """Write the manifest to *path*, preserving formatting and comments. @@ + self._normalize_string_scalars() + target = path or self.__path if not target: raise RuntimeError("Cannot dump manifest with no path")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dfetch/manifest/manifest.py` around lines 354 - 364, The dump method may write newly added/modified string scalars without the normalization applied at construction, causing values like "176" or "%feature" to be emitted unquoted; before writing in dump (method dump in class Manifest), call the existing normalization routine (e.g. _normalize_string_scalars) to re-normalize the live YAML document so string scalars get proper tagging/quoting; ensure this is applied right before manifest_file.write(self._doc.as_yaml()) so mutations from methods like append_project_entry are normalized prior to serializing.
182-184:⚠️ Potential issue | 🟠 MajorCheck uniqueness on the effective
dst, not just explicit keys.
_ensure_unique(projects_raw, "dst", ...)skips entries that omitdst, but those still resolve toname. A manifest containing- name: fooand- name: bar; dst: foopasses validation and still makes both projects target the same destination.♻️ Proposed fix
_ensure_unique(remotes_raw, "name", "manifest.remotes") _ensure_unique(projects_raw, "name", "manifest.projects") - _ensure_unique(projects_raw, "dst", "manifest.projects") + _ensure_unique( + [ + {"dst": project.get("dst", project["name"])} + for project in projects_raw + ], + "dst", + "manifest.projects", + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dfetch/manifest/manifest.py` around lines 182 - 184, The uniqueness check currently calls _ensure_unique(projects_raw, "dst", "manifest.projects") which ignores entries that lack an explicit "dst" (they effectively use their "name"), so compute the effective destination for each project (e.g., effective_dst = project.get("dst") or project["name"]) and run the uniqueness check against those effective values; update the validation to either build a list of effective dsts and call _ensure_unique on that list or modify _ensure_unique to accept a fallback key/resolver for projects_raw so that collisions between explicit dst and defaulted name are detected for manifest.projects.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dfetch/manifest/manifest.py`:
- Around line 395-403: The function _normalize_string_scalars currently only
wraps top-level mapping values under manifest_mu.get("projects") and
manifest_mu.get("remotes") with _yaml_str, missing nested strings (e.g.,
integrity.hash and list items like ignore); update _normalize_string_scalars to
recursively traverse mappings and sequences within each entry so every string
value encountered is passed to _yaml_str (not just direct values), preserving
existing calls for project and remote entries while ensuring nested dict keys
and list items are normalized too.
---
Outside diff comments:
In `@tests/test_manifest.py`:
- Around line 420-425: The test currently calls manifest._doc.as_yaml() instead
of the public API; change test_dump_writes_version_as_quoted_string to call
Manifest.dump() and capture its output using mock_open (or a tmp file fixture)
so you assert against what dump actually writes; locate the test function
test_dump_writes_version_as_quoted_string and replace the _doc.as_yaml() usage
with a call to manifest.dump() while using mock_open()/patch on builtins.open
(or the test's tmp file writer) to read the written content and assert that
"version: '<manifest.version>'" appears in that captured output.
---
Duplicate comments:
In `@dfetch/manifest/manifest.py`:
- Around line 354-364: The dump method may write newly added/modified string
scalars without the normalization applied at construction, causing values like
"176" or "%feature" to be emitted unquoted; before writing in dump (method dump
in class Manifest), call the existing normalization routine (e.g.
_normalize_string_scalars) to re-normalize the live YAML document so string
scalars get proper tagging/quoting; ensure this is applied right before
manifest_file.write(self._doc.as_yaml()) so mutations from methods like
append_project_entry are normalized prior to serializing.
- Around line 182-184: The uniqueness check currently calls
_ensure_unique(projects_raw, "dst", "manifest.projects") which ignores entries
that lack an explicit "dst" (they effectively use their "name"), so compute the
effective destination for each project (e.g., effective_dst = project.get("dst")
or project["name"]) and run the uniqueness check against those effective values;
update the validation to either build a list of effective dsts and call
_ensure_unique on that list or modify _ensure_unique to accept a fallback
key/resolver for projects_raw so that collisions between explicit dst and
defaulted name are detected for manifest.projects.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4b29e911-e230-4cae-8472-14419d10b4ad
📒 Files selected for processing (4)
dfetch/commands/import_.pydfetch/manifest/manifest.pytests/test_import.pytests/test_manifest.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dfetch/manifest/manifest.py`:
- Around line 594-614: The substitution in _render that inserts blank lines
between list entries is too permissive (parts loop using re.sub(r"\n( - )",
...)), which can match ` - ` inside multi-line scalar values; tighten the regex
to only target actual sequence-item line starts—e.g. replace that re.sub with
one using a multiline/anchored pattern such as re.sub(r"(?m)^\s{2}- ", r"\n\n -
", parts[i]) or use a lookahead like r"\n( - (?=\S))" so only true list-item
lines in the parts variable are matched and modified.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2286969b-260f-4832-8225-5d4b057e7ee2
📒 Files selected for processing (2)
dfetch/manifest/manifest.pytests/test_manifest.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Improvements
Bug Fixes