feat: Make Delete Operator Strict, Improve Error Messages, Remove Color Formatting, and Make Local Raw References Lazy

[ibro45]

Description

This PR includes several improvements to error handling, operator behavior, and code simplification:

• Delete operator now errors on missing keys: Changed the ~ delete operator from idempotent (silently skip missing keys) to strict (error if key doesn't exist). This provides better feedback when trying to delete non-existent keys and makes configurations more predictable.
• Improved error message readability: Restructured error messages to put the actual error text first, followed by location information in a separate line. This makes errors much easier to read and parse.
• Simplified instantiation error messages: Removed verbose details about arguments and parameters in favor of clearer, more concise error messages that focus on the root cause.
• Removed color formatting module: Dropped the entire formatters.py module and related color utilities (enable_colors, format_error, format_code, etc.) as they were not essential to the core functionality.
• Cleaned up public API: Removed color-related exports from __init__.py files to simplify the public interface.
• Updated documentation: Removed references to "idempotent" behavior throughout docs and updated operator descriptions.

Error Message Format Changes

Before:

[config.yaml:10 → model.encoder] Failed to instantiate 'invalid.module': ...

After:

Failed to instantiate 'invalid.module': ...

[config.yaml:10 → model.encoder]:

  5 | model:
  6 |   _target_: invalid.module
  7 |   encoder:

Related Issue

N/A

Type of Change

• 📚 Examples / docs / tutorials / dependencies update
• 🔧 Bug fix (non-breaking change which fixes an issue)
• 🥂 Improvement (non-breaking change which improves an existing feature)
• 🚀 New feature (non-breaking change which adds functionality)
• 💥 Breaking change (fix or feature that would cause existing functionality to change)
• 🔐 Security fix

Checklist

• I've read the CODE_OF_CONDUCT.md document.
• I've read the CONTRIBUTING.md guide.
• I've updated the code style using make codestyle.
• I've written tests for all new methods and classes that I created.
• I've written the docstring in Google format for all the methods and classes that I used.

[codecov]

Codecov Report

❌ Patch coverage is 95.79439% with 9 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/sparkwheel/utils/exceptions.py	83.33%	1 Missing and 3 partials ⚠️
src/sparkwheel/config.py	93.33%	1 Missing and 2 partials ⚠️
src/sparkwheel/loader.py	93.33%	0 Missing and 1 partial ⚠️
src/sparkwheel/preprocessor.py	96.96%	0 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[ibro45]

@CodeRabbit review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Walkthrough

Renames SourceLocation → Location and MetadataRegistry → LocationRegistry and adds src/sparkwheel/locations.py. Threads LocationRegistry instances through loader, parser, config, schema, operators, preprocessor, and tests. Changes delete operator semantics to raise on missing keys and adds MergeContext for location-aware merges. Introduces build_missing_key_error and new location-aware error types (ConfigKeyError, ConfigMergeError, CircularReferenceError). Removes the color formatter module and related public exports. Adds get_by_id path utility and SIMILARITY_THRESHOLD constant. Adds a GitHub Actions release workflow. Extensive tests added/updated for operators, preprocessor, path utils, and other behaviors. Documentation and README updated to reflect new semantics.

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

• Pay extra attention to:
  • src/sparkwheel/preprocessor.py: two-phase raw-ref expansion, external_only behavior, loader.load_file usage, and new exception flows.
  • src/sparkwheel/config.py: LocationRegistry propagation, MergeContext threading, changed merge/update semantics, and non-idempotent delete error paths.
  • src/sparkwheel/operators.py: MergeContext API, apply_operators signature change, and delete operator validation/error construction.
  • src/sparkwheel/utils/exceptions.py: Location class semantics, build_missing_key_error logic, and exported exception types.
  • Removal of src/sparkwheel/errors/formatters.py and related export updates: search for any remaining references to removed functions/constants.
  • New modules: src/sparkwheel/locations.py and src/sparkwheel/path_utils.py (get_by_id) for API correctness and error message content.
  • Tests: tests/test_operators.py, tests/test_preprocessor.py, tests/test_path_utils.py and related updates verifying error types, messages, and location assertions.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description check	✅ Passed	Description covers all key changes with clear examples and properly documents breaking changes. However, Related Issue section is marked N/A and some checklist items may need verification.
Docstring Coverage	✅ Passed	Docstring coverage is 95.58% which is sufficient. The required threshold is 80.00%.
Title check	✅ Passed	The title accurately summarizes the main changes: delete operator strictness, error message improvements, color formatting removal, and lazy local raw references.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch improve

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/sparkwheel/items.py (1)

328-343: Expression.evaluate docstring’s raised-exception type is outdated

The docstring still claims RuntimeError on failure, but the code raises EvaluationError. Adjust the “Raises” section to mention EvaluationError instead.

Also applies to: 360-364

🧹 Nitpick comments (12)

src/sparkwheel/utils/constants.py (1)

1-17: SIMILARITY_THRESHOLD constant looks good; consider using it in tests

Constant name, value, and comment are clear. To avoid magic numbers drifting, consider importing SIMILARITY_THRESHOLD in suggestion-related tests (e.g. tests/test_error_messages.py) instead of hardcoding 0.6 there.

tests/test_config.py (1)

632-641: Non-existent-key delete test matches new strict semantics; consider also covering Config.update path

The test correctly asserts that ~b on a base without b now raises ConfigMergeError with the expected message. For end-to-end coverage, you might also add a similar test that goes through Config.update("~b") to ensure the CLI/override path surfaces the same error.

src/sparkwheel/utils/exceptions.py (2)

21-41: Location dataclass is fine; add a negative test for line validation

The location model and __post_init__ guard (line >= 1) are reasonable. Given the explicit validation, consider adding a focused test that constructing Location(..., line=0, ...) raises ValueError to cover this branch.

---

66-93: Update _format_message docstring/comment to match new layout

The implementation now outputs the error text first, then the [file:line → id] line, but the docstring and inline comment still describe putting file:line on the first line. Update those descriptions so they match the actual (and desired) format.

src/sparkwheel/items.py (1)

211-224: InstantiationError wrapping is acceptable but could include minimal context

Catching generic exceptions and re‑raising InstantiationError(str(e), source_location=...) meets the goal of simpler messages. If you ever want slightly more context without the old verbosity, consider including the resolved target (e.g., f\"Failed to instantiate {modname}: {e}\"), but that’s optional.

tests/test_loader.py (1)

239-320: LocationRegistry tests are good; consider adding a bad-line test

The registry behavior (register/get, len, contains, merge, copy) is well covered. To exercise the Location validation branch, you could add a small test that attempts Location(filepath=\"config.yaml\", line=0, ...) and asserts ValueError.

src/sparkwheel/locations.py (3)

5-5: Consider re-exporting Location for convenience.

Users working with LocationRegistry will need Location. Re-exporting it here saves an import.

-__all__ = ["LocationRegistry"]
+__all__ = ["LocationRegistry", "Location"]

---

26-28: Add type hint to __init__ parameter.

-    def __init__(self):
+    def __init__(self) -> None:

---

68-74: Consider adding __repr__ for debugging.

A __repr__ showing registered paths aids debugging.

+    def __repr__(self) -> str:
+        """Return string representation of registry."""
+        return f"LocationRegistry({len(self._locations)} locations)"
+
     def __len__(self) -> int:

src/sparkwheel/config.py (1)

488-493: Redundant if self._locations check.

_locations is always initialized in __init__, so the conditional is unnecessary.

-                    source_location = self._locations.get(actual_key) if self._locations else None
+                    source_location = self._locations.get(actual_key)

Static analysis flagged lines 490-492 as uncovered. Add a test for delete operator on missing key.

src/sparkwheel/loader.py (2)

136-149: Docstring return description is stale.

Says "metadata_registry" but type hint says LocationRegistry.

         Returns:
-            Tuple of (config_dict, metadata_registry)
+            Tuple of (config_dict, location_registry)

---

178-184: Add missing return type hint.

-    def _load_yaml_with_metadata(self, stream, filepath: str, registry: LocationRegistry) -> dict[str, Any]:  # type: ignore[no-untyped-def]
+    def _load_yaml_with_metadata(self, stream: Any, filepath: str, registry: LocationRegistry) -> dict[str, Any]:

The # type: ignore comment suggests stream needs a type. Use Any or typing.IO[str].

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between 936bfc2 and 196c497.

⛔ Files ignored due to path filters (1)

• uv.lock is excluded by !**/*.lock

📒 Files selected for processing (26)

• .github/workflows/release.yml (1 hunks)
• README.md (1 hunks)
• docs/index.md (1 hunks)
• docs/user-guide/advanced.md (0 hunks)
• docs/user-guide/cli.md (1 hunks)
• docs/user-guide/operators.md (3 hunks)
• pyproject.toml (0 hunks)
• src/sparkwheel/__init__.py (2 hunks)
• src/sparkwheel/config.py (11 hunks)
• src/sparkwheel/errors/__init__.py (0 hunks)
• src/sparkwheel/errors/formatters.py (0 hunks)
• src/sparkwheel/items.py (5 hunks)
• src/sparkwheel/loader.py (10 hunks)
• src/sparkwheel/locations.py (1 hunks)
• src/sparkwheel/metadata.py (0 hunks)
• src/sparkwheel/operators.py (7 hunks)
• src/sparkwheel/parser.py (3 hunks)
• src/sparkwheel/schema.py (4 hunks)
• src/sparkwheel/utils/constants.py (2 hunks)
• src/sparkwheel/utils/exceptions.py (6 hunks)
• src/sparkwheel/utils/module.py (1 hunks)
• tests/test_config.py (1 hunks)
• tests/test_error_messages.py (1 hunks)
• tests/test_items.py (5 hunks)
• tests/test_loader.py (9 hunks)
• tests/test_operators.py (1 hunks)

💤 Files with no reviewable changes (5)

• docs/user-guide/advanced.md
• src/sparkwheel/metadata.py
• pyproject.toml
• src/sparkwheel/errors/init.py
• src/sparkwheel/errors/formatters.py

🧰 Additional context used

📓 Path-based instructions (2)

**/*.py

⚙️ CodeRabbit configuration file

Review the Python code for quality and correctness. Ensure variable names adhere to PEP8 style guides, are sensible and informative in regards to their function, though permitting simple names for loop and comprehension variables. Ensure routine names are meaningful in regards to their function and use verbs, adjectives, and nouns in a semantically appropriate way. Docstrings should be present for all definition which describe each variable, return value, and raised exception in the appropriate section of the Google-style of docstrings. Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these. Suggest any enhancements for code improving efficiency, maintainability, comprehensibility, and correctness. Ensure new or modified definitions will be covered by existing or new unit tests.

Files:

• src/sparkwheel/utils/module.py
• src/sparkwheel/utils/constants.py
• src/sparkwheel/config.py
• src/sparkwheel/schema.py
• tests/test_config.py
• src/sparkwheel/loader.py
• tests/test_items.py
• tests/test_loader.py
• src/sparkwheel/operators.py
• src/sparkwheel/utils/exceptions.py
• tests/test_error_messages.py
• tests/test_operators.py
• src/sparkwheel/__init__.py
• src/sparkwheel/locations.py
• src/sparkwheel/parser.py
• src/sparkwheel/items.py

**/*.md

⚙️ CodeRabbit configuration file

Remember that documentation must be updated with the latest information.

Files:

• docs/index.md
• docs/user-guide/operators.md
• docs/user-guide/cli.md
• README.md

🧬 Code graph analysis (12)

src/sparkwheel/schema.py (1)

src/sparkwheel/utils/exceptions.py (2)

• BaseError (46-122)
• Location (22-43)

tests/test_config.py (2)

src/sparkwheel/utils/exceptions.py (1)

• ConfigMergeError (201-210)

src/sparkwheel/operators.py (1)

• apply_operators (186-365)

tests/test_items.py (2)

src/sparkwheel/utils/exceptions.py (2)

• InstantiationError (137-140)
• Location (22-43)

src/sparkwheel/items.py (1)

• resolve_args (159-185)

tests/test_loader.py (3)

src/sparkwheel/locations.py (2)

• LocationRegistry (8-74)
• get (39-48)

src/sparkwheel/utils/exceptions.py (1)

• Location (22-43)

src/sparkwheel/loader.py (2)

• Loader (116-241)
• load_file (136-176)

src/sparkwheel/operators.py (3)

src/sparkwheel/config.py (2)

• locations (363-374)
• get (206-261)

src/sparkwheel/locations.py (2)

• LocationRegistry (8-74)
• get (39-48)

src/sparkwheel/utils/exceptions.py (3)

• Location (22-43)
• ConfigMergeError (201-210)
• build_missing_key_error (235-288)

src/sparkwheel/utils/exceptions.py (1)

src/sparkwheel/errors/suggestions.py (1)

• get_suggestions (51-98)

tests/test_error_messages.py (1)

src/sparkwheel/utils/exceptions.py (2)

• Location (22-43)
• BaseError (46-122)

tests/test_operators.py (4)

src/sparkwheel/config.py (1)

• locations (363-374)

src/sparkwheel/locations.py (2)

• LocationRegistry (8-74)
• register (30-37)

src/sparkwheel/operators.py (3)

• MergeContext (18-94)
• child_path (42-58)
• get_source_location (60-94)

src/sparkwheel/utils/exceptions.py (2)

• ConfigMergeError (201-210)
• Location (22-43)

src/sparkwheel/__init__.py (1)

src/sparkwheel/utils/exceptions.py (1)

• Location (22-43)

src/sparkwheel/locations.py (1)

src/sparkwheel/utils/exceptions.py (1)

• Location (22-43)

src/sparkwheel/parser.py (2)

src/sparkwheel/config.py (1)

• locations (363-374)

src/sparkwheel/locations.py (1)

• LocationRegistry (8-74)

src/sparkwheel/items.py (1)

src/sparkwheel/utils/exceptions.py (4)

• EvaluationError (213-216)
• InstantiationError (137-140)
• Location (22-43)
• TargetNotFoundError (125-128)

🪛 GitHub Check: codecov/patch

src/sparkwheel/config.py

[warning] 490-492: src/sparkwheel/config.py#L490-L492
Added lines #L490 - L492 were not covered by tests

---

[warning] 536-536: src/sparkwheel/config.py#L536
Added line #L536 was not covered by tests

src/sparkwheel/utils/exceptions.py

[warning] 40-40: src/sparkwheel/utils/exceptions.py#L40
Added line #L40 was not covered by tests

🔇 Additional comments (37)

docs/index.md (1)

195-214: Front-page description of ~ matches new strict delete semantics

Dropping the "idempotent" wording in favor of "Remove inherited keys explicitly" aligns with the new error-on-missing behavior while keeping the overview concise.

README.md (1)

64-75: Delete-operator rows in feature table clearly document behavior

The added rows for delete semantics (keys, list items, dict keys) clearly communicate Sparkwheel’s capabilities and match the documented ~ behavior elsewhere in the docs.

docs/user-guide/cli.md (1)

110-147: CLI override doc correctly notes delete now errors on missing keys

The ~key row explicitly stating “errors if missing” is consistent with the stricter delete semantics implemented in the operators layer and gives users the right expectation.

src/sparkwheel/utils/module.py (1)

203-253: InstantiationError message simplification is reasonable and preserves cause

The new error_msg format is concise, still includes the failing component path and original exception type/message, and fits cleanly into the existing InstantiationError/BaseError formatting pipeline.

tests/test_error_messages.py (1)

425-455: Location-based error tests correctly exercise new formatting

Switching to Location and asserting str(Location) plus BaseError output (with and without readable files) matches the new error-formatting behaviors and gives good coverage of edge cases.

.github/workflows/release.yml (1)

1-36: Release workflow and main-branch ancestry guard look correct

The workflow only runs for tag pushes, fetches full history, and correctly checks that the tagged commit is reachable from origin/main before creating a release, which enforces the intended “releases from main only” policy.

src/sparkwheel/__init__.py (1)

13-23: Public API swap to Location looks consistent

Import and __all__ now expose Location instead of the old location type; surface matches the new exceptions module. This aligns with the PR’s API direction and looks good.

Also applies to: 27-57

src/sparkwheel/parser.py (1)

5-7: Parser’s switch to LocationRegistry is coherent

Type hints, example, and internal usage of LocationRegistry are aligned and keep behavior unchanged; using metadata as the parameter name is acceptable given existing calling code.

Also applies to: 30-47, 49-57

tests/test_items.py (3)

190-199: Relaxed InstantiationError assertion matches new messaging

Asserting only the exception type (not the message) is appropriate now that instantiation errors are intentionally simplified.

---

286-296: resolve_args kwargs-only check is fine

Binding only kwargs from resolve_args() keeps the test focused on exclusion of special keys and matches the helper’s contract.

---

457-497: Location-based item/component/expression tests are solid

The new TestItemWithLocation cases correctly verify that Location objects round‑trip through items and are attached to errors, covering the Location integration nicely.

docs/user-guide/operators.md (2)

503-513: Hydra comparison table and bullets match the current operator set

The table and follow‑up bullets accurately reflect the reduced operator set and YAML+CLI support; they’re consistent with the rest of the doc after the delete‑semantics fix above.

---

282-286: Programmatic delete example matches strict ~ semantics

The Python example for deleting keys with ~training::old_param / ~model::dropout reads correctly under the new strict delete behavior.

src/sparkwheel/utils/exceptions.py (3)

143-199: ConfigKeyError suggestion plumbing still coherent after Location switch

Using Location | None here fits the new location type, and the suggestion generation still delegates cleanly to the shared formatting/suggestion helpers. No issues spotted.

---

201-210: ConfigMergeError docstring now correctly calls out ~ misuse

The class docstring explicitly documents the new strict delete semantics and other merge failures; matches the PR behavior.

---

235-288: build_missing_key_error nicely centralizes delete-missing diagnostics

The helper composes clear messages for both top‑level and nested deletes and folds in typo suggestions and available-key hints; this is a good abstraction for delete failures.

tests/test_operators.py (4)

16-131: MergeContext tests cover the important path and operator cases

The tests for child_path and get_source_location (including operator stripping and precedence) thoroughly exercise the context logic and registry interactions.

---

133-167: Delete-operator validation tests match the intended value constraints

Allowing None, empty string, and non‑empty lists while rejecting non‑empty strings, dicts, ints, and empty lists is consistent and clearly encoded in this test block.

---

215-354: apply_operators edge-case tests look comprehensive

These cases cover non-dict inputs, context propagation, strict delete behavior (with and without context), list/dict composition, scalar replacement, and multiple operators in one override. This gives strong confidence in the new operator semantics.

---

356-395: Deep-copy behavior tests are appropriate

Verifying that base, override, and result structures remain independent is important here; these tests cover that contract well.

src/sparkwheel/items.py (1)

8-11: Location plumbing through Item / Component / Expression is consistent

Using Location | None for source_location across these constructors aligns with the new exception/location model and the tests that assert location propagation.

Also applies to: 49-53, 129-131, 299-307

tests/test_loader.py (1)

5-8: Loader tests match the new LocationRegistry-based API

Asserting that metadata objects are LocationRegistry instances (and using them across single and multi-file loads) correctly reflects the updated loader contract.

Also applies to: 18-23, 27-31, 35-39, 133-140

src/sparkwheel/config.py (3)

186-186: LGTM!

Clean initialization of LocationRegistry.

---

362-374: LGTM!

Property exposes the registry cleanly with good docstring.

---

534-542: Line 536 lacks test coverage per static analysis.

Ensure tests cover the file-loading path with nested paths that merge location metadata.

src/sparkwheel/operators.py (5)

17-41: LGTM!

MergeContext dataclass is well-designed with clear docstrings and examples.

---

42-58: LGTM!

Path concatenation logic is correct.

---

60-94: LGTM!

Handles operator prefixes (~, =) correctly when looking up locations.

---

272-276: Delete operator now errors on missing keys.

This is a breaking change per PR objectives. Error message includes source location for better debugging.

---

348-351: LGTM!

Context propagation for recursive merging is correct.

src/sparkwheel/schema.py (3)

42-42: LGTM!

Import updated to use Location.

---

195-195: LGTM!

Type hint correctly updated to Location | None.

---

685-703: LGTM!

Return type and docstring updated consistently. The isinstance check on line 702 is defensive but acceptable.

src/sparkwheel/loader.py (4)

10-14: LGTM!

Imports updated for the Location/LocationRegistry migration.

---

19-30: LGTM!

Docstring updated to reflect LocationRegistry usage.

---

56-66: LGTM!

Key-level location tracking added. This enables precise error reporting for individual keys.

---

221-234: LGTM!

load_files correctly merges location registries.

[coderabbitai]

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/sparkwheel/preprocessor.py (1)

113-119: Same type annotation issue.

Apply consistent fix for locations parameter here as well.

🧹 Nitpick comments (3)

tests/test_preprocessor.py (2)

100-104: Assertion logic is confusing.

The condition "in '" not in error_msg or "in '10'" not in error_msg always passes if "in '" is absent. Consider simplifying:

-        assert "in '" not in error_msg or "in '10'" not in error_msg
+        assert "in '" not in error_msg

---

120-125: Tests rely on internal _original_message attribute.

Accessing _original_message couples tests to internal implementation. If the attribute name changes, multiple tests break. Consider using str(error) or exposing a public API if this is intentional test behavior.

src/sparkwheel/preprocessor.py (1)

342-344: Consider clearer truncation format.

Current output: Available keys: ['key1', ...]... may look awkward. Consider:

-                    error_msg += f". Available keys: {available_keys[:10]}"
-                    if len(available_keys) > 10:
-                        error_msg += "..."
+                    if len(available_keys) > 10:
+                        error_msg += f". Available keys (first 10 of {len(available_keys)}): {available_keys[:10]}"
+                    else:
+                        error_msg += f". Available keys: {available_keys}"

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between 196c497 and e26586c.

📒 Files selected for processing (3)

• src/sparkwheel/config.py (12 hunks)
• src/sparkwheel/preprocessor.py (11 hunks)
• tests/test_preprocessor.py (3 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

**/*.py

⚙️ CodeRabbit configuration file

Review the Python code for quality and correctness. Ensure variable names adhere to PEP8 style guides, are sensible and informative in regards to their function, though permitting simple names for loop and comprehension variables. Ensure routine names are meaningful in regards to their function and use verbs, adjectives, and nouns in a semantically appropriate way. Docstrings should be present for all definition which describe each variable, return value, and raised exception in the appropriate section of the Google-style of docstrings. Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these. Suggest any enhancements for code improving efficiency, maintainability, comprehensibility, and correctness. Ensure new or modified definitions will be covered by existing or new unit tests.

Files:

• tests/test_preprocessor.py
• src/sparkwheel/config.py
• src/sparkwheel/preprocessor.py

🧬 Code graph analysis (3)

tests/test_preprocessor.py (3)

src/sparkwheel/utils/exceptions.py (2)

• CircularReferenceError (131-134)
• ConfigKeyError (143-198)

src/sparkwheel/config.py (1)

• _get_by_id (670-694)

src/sparkwheel/loader.py (2)

• load_file (136-176)
• Loader (116-241)

src/sparkwheel/config.py (4)

src/sparkwheel/locations.py (3)

• LocationRegistry (8-74)
• merge (50-56)
• get (39-48)

src/sparkwheel/operators.py (1)

• MergeContext (18-94)

src/sparkwheel/parser.py (1)

• Parser (12-100)

src/sparkwheel/utils/exceptions.py (2)

• ConfigKeyError (143-198)
• build_missing_key_error (235-288)

src/sparkwheel/preprocessor.py (4)

src/sparkwheel/utils/exceptions.py (2)

• CircularReferenceError (131-134)
• ConfigKeyError (143-198)

src/sparkwheel/config.py (2)

• locations (363-374)
• get (206-261)

src/sparkwheel/loader.py (1)

• load_file (136-176)

src/sparkwheel/path_utils.py (1)

• split_id (251-268)

🪛 GitHub Actions: CI

src/sparkwheel/preprocessor.py

[error] 62-62: mypy: Function is missing a type annotation for one or more arguments in process_raw_refs.

---

[error] 67-67: mypy: Unused type: ignore comment at locations=None, # type: ignore[no-untyped-def] in process_raw_refs.

---

[error] 113-113: mypy: Function is missing a type annotation for one or more arguments in _process_raw_refs_recursive.

---

[error] 119-119: mypy: Unused type: ignore comment at locations=None, # type: ignore[no-untyped-def] in _process_raw_refs_recursive.

---

[error] 206-206: mypy: Function is missing a type annotation for one or more arguments in _expand_raw_ref.

---

[error] 212-212: mypy: Unused type: ignore comment at locations=None, # type: ignore[no-untyped-def] in _expand_raw_ref.

🪛 GitHub Check: codecov/patch

src/sparkwheel/config.py

[warning] 490-492: src/sparkwheel/config.py#L490-L492
Added lines #L490 - L492 were not covered by tests

---

[warning] 536-536: src/sparkwheel/config.py#L536
Added line #L536 was not covered by tests

src/sparkwheel/preprocessor.py

[warning] 152-152: src/sparkwheel/preprocessor.py#L152
Added line #L152 was not covered by tests

🔇 Additional comments (6)

tests/test_preprocessor.py (2)

1-26: LGTM!

Test setup is correct. The test properly validates circular reference detection using the new CircularReferenceError and passes locations to process_raw_refs.

---

219-259: LGTM!

Success path tests for raw reference expansion are well-structured and cover intra-config, nested paths, and external file references.

src/sparkwheel/preprocessor.py (3)

149-152: Missing test coverage for list processing with locations.

Line 152 is not covered by tests per static analysis. Add a test with a list containing raw references to ensure location propagation works correctly for lists.

---

229-241: LGTM!

Circular reference detection with location-aware error reporting is well-implemented.

---

309-365: LGTM overall!

The _get_by_id method now provides contextual error messages with parent paths and available keys. Logic is correct.

src/sparkwheel/config.py (1)

362-374: LGTM - Clean public API addition.

The locations property provides appropriate read-only access to the LocationRegistry for external inspection of source locations.

[coderabbitai]

Actionable comments posted: 0

♻️ Duplicate comments (6)

src/sparkwheel/preprocessor.py (1)

65-94: LGTM - Type annotations properly addressed.

The locations parameter now uses Optional["LocationRegistry"] with TYPE_CHECKING, resolving the previous mypy issues. Docstring properly documents the parameter.

src/sparkwheel/config.py (1)

557-563: Test coverage for location merging added.

Past review comment requested test coverage for nested path location merging. The test test_update_from_file_with_nested_paths_merges_locations at lines 433-460 in test_config.py addresses this.

tests/test_operators.py (1)

206-213: LGTM - Comment accurately describes test behavior.

Past review comment about contradictory comments appears already addressed. The test and comment correctly indicate that dict values under remove operators are invalid.

docs/user-guide/operators.md (2)

134-136: Clear documentation of strict delete semantics.

The warning correctly documents that delete raises errors for missing keys, addressing the past review comment about idempotent claims.

---

438-470: Thorough guidance on strict delete semantics.

This section properly documents the strict delete behavior and provides practical alternatives for reusable configs. Addresses the past review comment about removing idempotent claims.

tests/test_config.py (1)

433-460: Test addresses past review comment on location merging.

This test properly verifies that nested-path syntax in YAML files correctly tracks locations, directly addressing the previous review comment about line 557 coverage.

🧹 Nitpick comments (1)

src/sparkwheel/preprocessor.py (1)

334-349: Consider clarifying truncation indicator.

Lines 345-347 truncate the available keys list, but the "..." suffix could be clearer about how many keys were omitted.

-                    error_msg += f". Available keys: {available_keys[:10]}"
-                    if len(available_keys) > 10:
-                        error_msg += "..."
+                    shown_keys = available_keys[:10]
+                    error_msg += f". Available keys: {shown_keys}"
+                    if len(available_keys) > 10:
+                        error_msg += f" (and {len(available_keys) - 10} more)"

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between e26586c and b3516ea.

📒 Files selected for processing (5)

• docs/user-guide/operators.md (4 hunks)
• src/sparkwheel/config.py (12 hunks)
• src/sparkwheel/preprocessor.py (11 hunks)
• tests/test_config.py (3 hunks)
• tests/test_operators.py (1 hunks)

🧰 Additional context used

📓 Path-based instructions (2)

**/*.md

⚙️ CodeRabbit configuration file

Remember that documentation must be updated with the latest information.

Files:

• docs/user-guide/operators.md

**/*.py

⚙️ CodeRabbit configuration file

Review the Python code for quality and correctness. Ensure variable names adhere to PEP8 style guides, are sensible and informative in regards to their function, though permitting simple names for loop and comprehension variables. Ensure routine names are meaningful in regards to their function and use verbs, adjectives, and nouns in a semantically appropriate way. Docstrings should be present for all definition which describe each variable, return value, and raised exception in the appropriate section of the Google-style of docstrings. Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these. Suggest any enhancements for code improving efficiency, maintainability, comprehensibility, and correctness. Ensure new or modified definitions will be covered by existing or new unit tests.

Files:

• tests/test_operators.py
• src/sparkwheel/preprocessor.py
• tests/test_config.py
• src/sparkwheel/config.py

🧬 Code graph analysis (4)

tests/test_operators.py (4)

src/sparkwheel/config.py (1)

• locations (363-374)

src/sparkwheel/locations.py (2)

• LocationRegistry (8-74)
• register (30-37)

src/sparkwheel/operators.py (6)

• MergeContext (18-94)
• _validate_delete_operator (97-139)
• apply_operators (186-365)
• validate_operators (142-183)
• child_path (42-58)
• get_source_location (60-94)

src/sparkwheel/utils/exceptions.py (2)

• ConfigMergeError (201-210)
• Location (22-43)

src/sparkwheel/preprocessor.py (6)

src/sparkwheel/path_utils.py (4)

• resolve_relative_ids (289-352)
• split_file_and_id (134-163)
• split_file_and_id (218-231)
• split_id (251-268)

src/sparkwheel/resolver.py (1)

• split_id (295-308)

src/sparkwheel/utils/exceptions.py (2)

• CircularReferenceError (131-134)
• ConfigKeyError (143-198)

src/sparkwheel/config.py (2)

• locations (363-374)
• get (206-261)

src/sparkwheel/locations.py (1)

• LocationRegistry (8-74)

src/sparkwheel/loader.py (1)

• load_file (136-176)

tests/test_config.py (2)

src/sparkwheel/config.py (3)

• Config (113-783)
• update (376-458)
• get (206-261)

src/sparkwheel/utils/exceptions.py (1)

• ConfigMergeError (201-210)

src/sparkwheel/config.py (5)

src/sparkwheel/locations.py (3)

• LocationRegistry (8-74)
• merge (50-56)
• get (39-48)

src/sparkwheel/operators.py (1)

• MergeContext (18-94)

src/sparkwheel/parser.py (1)

• Parser (12-100)

src/sparkwheel/preprocessor.py (1)

• process_raw_refs (65-94)

src/sparkwheel/utils/exceptions.py (2)

• ConfigKeyError (143-198)
• build_missing_key_error (235-288)

🪛 GitHub Check: codecov/patch

src/sparkwheel/preprocessor.py

[warning] 155-155: src/sparkwheel/preprocessor.py#L155
Added line #L155 was not covered by tests

src/sparkwheel/config.py

[warning] 511-511: src/sparkwheel/config.py#L511
Added line #L511 was not covered by tests

🪛 markdownlint-cli2 (0.18.1)

docs/user-guide/operators.md

443-443: Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)

---

444-444: Code block style
Expected: indented; Actual: fenced

(MD046, code-block-style)

---

454-454: Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)

---

455-455: Code block style
Expected: indented; Actual: fenced

(MD046, code-block-style)

---

464-464: Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)

---

465-465: Code block style
Expected: indented; Actual: fenced

(MD046, code-block-style)

🔇 Additional comments (7)

src/sparkwheel/preprocessor.py (1)

209-290: Enhanced error handling with location context.

The integration of LocationRegistry for error reporting is well-implemented. Particularly good:

• Lines 235-244: Circular reference errors now include source location
• Lines 262-280: Raw reference resolution failures provide detailed context
• Line 284: Correctly propagates loaded_locations for nested processing

src/sparkwheel/config.py (2)

362-374: LGTM - Clean API addition.

The locations property provides clean public access to the location registry with a helpful docstring example.

---

488-513: Based on my verification, the edge case is already tested. The test test_delete_nested_key_when_parent_doesnt_exist at line 954 in tests/test_config.py directly exercises the code path at line 511, where the parent container doesn't exist.

The test scenario:

• Config contains {"model": {"lr": 0.001}}
• Attempts to delete ~trainer::epochs (where trainer doesn't exist)
• This triggers the fallback at line 511 to show top-level keys
• Error message correctly shows the parent name and available keys

The implementation is correct and the edge case is well-covered by tests.

---

Edge case for deleting nested keys with non-existent parent is already tested.

Line 511 fallback (showing top-level keys when parent doesn't exist) is covered by the existing test test_delete_nested_key_when_parent_doesnt_exist at line 954 of tests/test_config.py. The implementation correctly identifies nested vs. top-level deletes and shows appropriate available keys.

tests/test_operators.py (1)

1-395: Excellent comprehensive test coverage.

This test file provides thorough coverage of the operators module:

• 50 test methods across 5 test classes
• Edge cases, error conditions, and normal operations covered
• Clear test names and docstrings
• Good organization by functionality

docs/user-guide/operators.md (1)

527-536: Accurate feature comparison.

The comparison table correctly highlights Sparkwheel's YAML operator support and simpler operator set compared to Hydra.

tests/test_config.py (2)

660-669: Test correctly updated for strict delete semantics.

The renamed test now properly verifies that deleting a non-existent key raises ConfigMergeError, matching the new strict delete behavior.

---

921-966: Comprehensive delete error message tests.

These three tests verify error message quality for different delete scenarios:

• Top-level key deletion (shows top-level keys)
• Nested key deletion (shows parent container keys)
• Nested key when parent doesn't exist (edge case)

The test at lines 954-966 appears to cover the edge case at line 511 in config.py (fallback to top-level keys when parent doesn't exist).

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (4)

src/sparkwheel/items.py (1)

221-227: Add test coverage for line 227.

The re-raise path when InstantiationError already has a location (line 227) is not covered by tests. Add a test case where instantiate() raises an InstantiationError that already contains a source_location.

src/sparkwheel/path_utils.py (1)

308-308: Type annotation is stricter than actual usage.

Tests at tests/test_path_utils.py:130-138 pass a string as config, but the annotation only allows dict[str, Any] | list[Any]. Either widen the annotation to Any or document that passing non-container types is expected to raise TypeError.

-def get_by_id(config: dict[str, Any] | list[Any], id: str) -> Any:
+def get_by_id(config: Any, id: str) -> Any:

Alternatively, update the docstring to clarify the behavior.

tests/test_preprocessor.py (1)

42-47: Tests access internal _original_message attribute.

Consider using str(error) or a public method if _original_message is an internal implementation detail. Otherwise, if this attribute is part of the API contract, document it in the exception class.

tests/test_config.py (1)

450-452: Consider tightening location assertions.

The or conditions are lenient. If the expected behavior is to track model::lr specifically, assert that directly. Otherwise, document why fallback to parent key is acceptable.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between b3516ea and 9ec8958.

📒 Files selected for processing (7)

• src/sparkwheel/config.py (15 hunks)
• src/sparkwheel/items.py (5 hunks)
• src/sparkwheel/path_utils.py (2 hunks)
• src/sparkwheel/preprocessor.py (10 hunks)
• tests/test_config.py (4 hunks)
• tests/test_path_utils.py (2 hunks)
• tests/test_preprocessor.py (2 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

**/*.py

⚙️ CodeRabbit configuration file

Review the Python code for quality and correctness. Ensure variable names adhere to PEP8 style guides, are sensible and informative in regards to their function, though permitting simple names for loop and comprehension variables. Ensure routine names are meaningful in regards to their function and use verbs, adjectives, and nouns in a semantically appropriate way. Docstrings should be present for all definition which describe each variable, return value, and raised exception in the appropriate section of the Google-style of docstrings. Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these. Suggest any enhancements for code improving efficiency, maintainability, comprehensibility, and correctness. Ensure new or modified definitions will be covered by existing or new unit tests.

Files:

• src/sparkwheel/items.py
• tests/test_path_utils.py
• tests/test_preprocessor.py
• tests/test_config.py
• src/sparkwheel/preprocessor.py
• src/sparkwheel/path_utils.py
• src/sparkwheel/config.py

🧬 Code graph analysis (7)

src/sparkwheel/items.py (1)

src/sparkwheel/utils/exceptions.py (4)

• EvaluationError (213-216)
• InstantiationError (137-140)
• Location (22-43)
• TargetNotFoundError (125-128)

tests/test_path_utils.py (1)

src/sparkwheel/path_utils.py (1)

• get_by_id (308-366)

tests/test_preprocessor.py (4)

src/sparkwheel/utils/exceptions.py (2)

• CircularReferenceError (131-134)
• ConfigKeyError (143-198)

src/sparkwheel/config.py (1)

• locations (363-374)

src/sparkwheel/loader.py (2)

• load_file (136-176)
• Loader (116-241)

src/sparkwheel/preprocessor.py (1)

• process_raw_refs (65-95)

tests/test_config.py (2)

src/sparkwheel/utils/exceptions.py (1)

• ConfigMergeError (201-210)

src/sparkwheel/operators.py (1)

• apply_operators (186-365)

src/sparkwheel/preprocessor.py (5)

src/sparkwheel/path_utils.py (3)

• get_by_id (308-366)
• split_file_and_id (135-164)
• split_file_and_id (219-232)

src/sparkwheel/utils/exceptions.py (2)

• CircularReferenceError (131-134)
• ConfigKeyError (143-198)

src/sparkwheel/config.py (2)

• locations (363-374)
• get (206-261)

src/sparkwheel/locations.py (1)

• LocationRegistry (8-74)

src/sparkwheel/loader.py (1)

• load_file (136-176)

src/sparkwheel/path_utils.py (1)

src/sparkwheel/resolver.py (1)

• split_id (295-308)

src/sparkwheel/config.py (6)

src/sparkwheel/locations.py (3)

• LocationRegistry (8-74)
• merge (50-56)
• get (39-48)

src/sparkwheel/operators.py (4)

• MergeContext (18-94)
• _validate_delete_operator (97-139)
• apply_operators (186-365)
• validate_operators (142-183)

src/sparkwheel/parser.py (1)

• Parser (12-100)

src/sparkwheel/path_utils.py (2)

• get_by_id (308-366)
• split_id (252-269)

src/sparkwheel/resolver.py (2)

• split_id (295-308)
• Resolver (16-430)

src/sparkwheel/utils/exceptions.py (2)

• ConfigKeyError (143-198)
• build_missing_key_error (235-288)

🪛 GitHub Check: codecov/patch

src/sparkwheel/items.py

[warning] 227-227: src/sparkwheel/items.py#L227
Added line #L227 was not covered by tests

src/sparkwheel/preprocessor.py

[warning] 156-156: src/sparkwheel/preprocessor.py#L156
Added line #L156 was not covered by tests

src/sparkwheel/config.py

[warning] 511-511: src/sparkwheel/config.py#L511
Added line #L511 was not covered by tests

🔇 Additional comments (30)

src/sparkwheel/items.py (5)

10-10: LGTM!

Import updated to use Location instead of SourceLocation, consistent with PR-wide refactoring.

---

49-49: LGTM!

Type hint correctly updated to Location | None.

---

129-129: LGTM!

Type hint correctly updated to Location | None.

---

229-229: LGTM!

Comment accurately describes the exception handling logic.

---

314-314: LGTM!

Type hint correctly updated to Location | None.

src/sparkwheel/path_utils.py (1)

290-305: LGTM!

Clean helper that correctly returns empty context for first-level errors.

tests/test_path_utils.py (1)

35-160: LGTM!

Comprehensive test coverage for get_by_id including edge cases: empty ID, list indexing, nested paths, error messages with context, and truncation of available keys.

tests/test_preprocessor.py (1)

107-147: LGTM!

Good coverage of success paths for raw reference expansion: local, nested, and external file references.

tests/test_config.py (3)

106-110: LGTM!

Test correctly updated to expect TypeError matching the new get_by_id behavior.

---

660-669: LGTM!

Test correctly updated for the breaking change: delete operator now raises ConfigMergeError on missing keys instead of silently skipping.

---

921-966: LGTM!

Thorough tests verifying that delete errors include helpful context: available keys from the appropriate container level.

src/sparkwheel/preprocessor.py (5)

9-16: LGTM!

Clean import organization with TYPE_CHECKING block for LocationRegistry to avoid circular imports.

---

65-95: LGTM!

Type annotation fixed per past review, docstring updated with locations parameter and new exception types.

---

154-157: Missing test coverage for list recursion with locations.

Static analysis indicates line 156 is untested. Add a test with raw references inside list items to cover this path.

Example test structure:

def test_raw_ref_in_list_with_location(self, tmp_path):
    config_file = tmp_path / "config.yaml"
    config_file.write_text('value: 42\nitems:\n  - "%value"')
    # ... verify expansion and location tracking

---

234-246: LGTM!

Circular reference errors now include source location when available, with clear reference chain in the message.

---

264-286: LGTM!

Error handling correctly wraps navigation failures in ConfigKeyError with location context. Uses the loaded file's location registry for nested references.

src/sparkwheel/config.py (14)

100-108: LGTM!

Import changes align with LocationRegistry refactoring and new operator/error utilities.

---

186-186: LGTM!

LocationRegistry initialization is correct.

---

362-374: LGTM!

Read-only property exposing LocationRegistry is a clean API addition.

---

462-464: LGTM!

MergeContext propagation and location merging are correctly implemented.

---

519-520: LGTM!

MergeContext with current_path enables location-aware recursive merging.

---

545-546: LGTM!

MergeContext usage is consistent with other update paths.

---

683-683: LGTM!

Parser correctly receives LocationRegistry for source location tracking.

---

701-704: LGTM!

Delegation to get_by_id utility centralizes logic and improves error messages.

---

260-260: LGTM!

Adding TypeError to exception handling is necessary since get_by_id raises TypeError for invalid indexing.

Also applies to: 752-752

---

332-332: LGTM!

Validation calls correctly pass LocationRegistry for location-aware error messages.

Also applies to: 453-453

---

443-443: LGTM!

Passing LocationRegistry to process_raw_refs enables better error reporting.

---

393-393: LGTM!

Documentation correctly reflects strict delete behavior.

---

557-557: Test coverage for nested path location merging already exists.

The test test_update_from_file_with_nested_paths_merges_locations (lines 433-453 in tests/test_config.py) directly validates the code at line 557. It creates a YAML file with nested-path syntax (::) and verifies both that values are correctly expanded and that location tracking via self._locations.merge(new_metadata) occurs. The past review's concern about missing test coverage has been addressed.

---

488-514: I'll verify the claims in this review comment by examining the test coverage and implementation details of the strict delete behavior.
<function_calls>

#!/bin/bash

Search for existing tests related to delete, strict, and missing key behavior

find . -name ".py" -path "/test*" -type f 2>/dev/null | head -20


</function_calls>

<function_calls>

#!/bin/bash

Find test files for config.py

fd -e py . tests/ 2>/dev/null | grep -E "config|delete" | head -20


</function_calls>

<function_calls>

#!/bin/bash

List all test files

ls -la tests/ 2>/dev/null || find . -type d -name tests 2>/dev/null | head -10


</function_calls>

<function_calls>

#!/bin/bash

Use git to find test files

git ls-files | grep -E "test.*.py$|.py.*test" | head -30


</function_calls>

<function_calls>

#!/bin/bash

Search for tests related to delete, strict, and missing keys

rg -n "test.*delete|def test.*strict|missing.*key|KeyError.*delete" --type=py -A 3 | head -100


</function_calls>

<function_calls>

#!/bin/bash

Look at the implementation of build_missing_key_error

rg -n "build_missing_key_error" --type=py -B 5 -A 10 | head -80


</function_calls>

<function_calls>

#!/bin/bash

Read the specific lines of interest from config.py

sed -n '480,520p' src/sparkwheel/config.py


</function_calls>

<function_calls>

#!/bin/bash

Check ID_SEP_KEY definition and _get_by_id

rg -n "ID_SEP_KEY\s*=|def _get_by_id" --type=py -A 5 src/sparkwheel/config.py | head -30


</function_calls>

<function_calls>

#!/bin/bash

Search for delitem or delete operation tests

rg -n "delitem|del\s+|delete.*nested|test.*delete" --type=py -B 2 -A 8 | grep -A 8 -B 2 "test" | head -120


</function_calls>

<function_calls>

#!/bin/bash

Find test files and show summary

find . -name "test_.py" -o -name "_test.py" 2>/dev/null | head -20


</function_calls>

<function_calls>

#!/bin/bash

List Python test files using git

git ls-files | grep -i test | grep ".py$" | head -20


</function_calls>

<function_calls>

#!/bin/bash

Get list of test files first, then search for delete tests

test_files=$(git ls-files | grep -i test | grep ".py$")
if [ -z "$test_files" ]; then
echo "No test files found via git ls-files"

Try alternative approach

fd -e py tests/ 2>/dev/null || echo "No tests directory found"
else
echo "Test files found:"
echo "$test_files"
echo ""
echo "Searching for delete-related tests:"
rg -n "def test.*delete|strict.*delete|delete.*strict" $test_files -A 5
fi


</function_calls>

<function_calls>

#!/bin/bash

Comprehensive search for tests related to delete

echo "=== Searching all Python files for delete tests ==="
rg -n "def test.*delete|def test.*strict|def test.*missing" --type=py -A 5 2>/dev/null | head -150


</function_calls>

<function_calls>

#!/bin/bash

Look at the full delitem method in config.py

rg -n "def delitem" --type=py -A 50 src/sparkwheel/config.py


</function_calls>

<function_calls>

#!/bin/bash

Get line count and exact location of delitem

rg -n "def delitem" --type=py src/sparkwheel/config.py


</function_calls>

<function_calls>

#!/bin/bash

Get exact lines 488-514 from config.py

echo "=== Lines 488-514 from config.py ==="
sed -n '488,514p' src/sparkwheel/config.py | cat -n
echo ""
echo "=== Checking for tests that use del operator or delitem ==="
rg -n "del\s+\w+[|delitem|test.[.]\s*=" --type=py -B 2 -A 2 2>/dev/null | head -100


</function_calls>

<function_calls>

#!/bin/bash

Find all Python test files in the repo

echo "=== All Python files in repo ==="
find . -name "*.py" -type f 2>/dev/null | grep -E "(test|spec)" | head -30


</function_calls>

<function_calls>

#!/bin/bash

Get all Python files and filter by test patterns more carefully

git ls-files "*.py" 2>/dev/null | sort


</function_calls>

<function_calls>

#!/bin/bash

Get all Python files

git ls-files "*.py" 2>/dev/null | grep -v pycache


</function_calls>

<function_calls>

#!/bin/bash

Count Python files and check for test directory

echo "=== Repository structure ==="
git ls-files | head -30


</function_calls>

<function_calls>

#!/bin/bash

Get complete file list and check for tests

git ls-files | wc -l
echo "---"
git ls-files | grep -i test


</function_calls>

<function_calls>

#!/bin/bash

Complete repo file list

git ls-files | sort


</function_calls>

<function_calls>

#!/bin/bash

Show repo structure more clearly

echo "=== Files in repo ==="
git ls-files | head -50
echo ""
echo "=== Any .py files containing 'test' in path ==="
git ls-files | grep -E "test.*.py|.py.*test"


</function_calls>

<function_calls>

#!/bin/bash

Let's focus on what we know: read the actual code from config.py

wc -l src/sparkwheel/config.py
echo ""
echo "=== Reading lines 480-520 ==="
sed -n '480,520p' src/sparkwheel/config.py


</function_calls>

<function_calls>

#!/bin/bash

Find build_missing_key_error definition

echo "=== build_missing_key_error function ==="
rg -n "def build_missing_key_error|build_missing_key_error" --type=py src/sparkwheel/config.py -B 2 -A 15 | head -50


</function_calls>

<function_calls>

#!/bin/bash

Search more carefully for the error building function

echo "=== Looking for build_missing_key_error ==="
rg "build_missing_key_error" --type=py -B 5 -A 10


</function_calls>

<function_calls>

#!/bin/bash

Check if there are test files at all

ls -la tests/ 2>/dev/null || echo "No tests/ directory"
ls -la test/ 2>/dev/null || echo "No test/ directory"
find . -type d -name "test" 2>/dev/null


</function_calls>

<function_calls>

#!/bin/bash

List directory structure

ls -la


</function_calls>

<function_calls>

#!/bin/bash

Complete listing of git files with focus on structure

echo "=== Git tracked files ==="
git ls-files


</function_calls>

<function_calls>

#!/bin/bash

Let's try a different approach - check the PR context

git log --oneline -20 2>/dev/null || echo "Git log not available"
git branch 2>/dev/null || echo "Git branch not available"


</function_calls>

<function_calls>

#!/bin/bash

Since we have the file, let's read more of it to understand the context

echo "=== Reading full delitem method ==="
sed -n '470,530p' src/sparkwheel/config.py | cat -n


</function_calls>

<function_calls>

#!/bin/bash

Search for helper functions like build_missing_key_error

echo "=== Searching for helper functions ==="
grep -n "def build_missing_key_error|def _get_by_id|def _delete_nested_key" src/sparkwheel/config.py


</function_calls>

<function_calls>

#!/bin/bash

Look for all function definitions to understand structure

echo "=== All function definitions ==="
grep -n "^def " src/sparkwheel/config.py | head -40


</function_calls>

<function_calls>

#!/bin/bash

Look for the specific helper functions

grep -n "build_missing_key_error|_get_by_id|_delete_nested_key" src/sparkwheel/config.py | head -20


</function_calls>

<function_calls>

#!/bin/bash

Search in broader scope - maybe these are imported or defined differently

echo "=== Searching for build_missing_key_error ==="
rg "build_missing_key_error" -B 3 -A 10 src/


</function_calls>

<function_calls>

#!/bin/bash

Let's read a larger section around line 488 to see imports and full context

sed -n '1,50p' src/sparkwheel/config.py
echo ""
echo "=== Lines 480-520 with more context ==="
sed -n '470,530p' src/sparkwheel/config.py


</function_calls>

<function_calls>

#!/bin/bash

Read the file to understand imports and structure

head -100 src/sparkwheel/config.py | cat -n


</function_calls>

<function_calls>

#!/bin/bash

Now let's search for where build_missing_key_error is defined

grep -n "build_missing_key_error" src/sparkwheel/config.py | head -5


</function_calls>

<function_calls>

#!/bin/bash

Let's look at what's actually at line 513 in the file

echo "=== Lines 510-515 ==="
sed -n '510,515p' src/sparkwheel/config.py | cat -n


</function_calls>

<function_calls>

#!/bin/bash

Get exact lines with context

sed -n '485,520p' src/sparkwheel/config.py | nl -v 485


</function_calls>

<function_calls>

#!/bin/bash

Now let's find all usages of build_missing_key_error to understand what it is

echo "=== All occurrences of build_missing_key_error ==="
grep -rn "build_missing_key_error" src/


</function_calls>

<function_calls>

#!/bin/bash

Let's search across the entire source directory

echo "=== File list in src/ ==="
find src/ -type f -name "*.py" | sort


</function_calls>

<function_calls>

#!/bin/bash

Now search in all source files

find src/ -name "*.py" -exec grep -l "build_missing_key_error" {} ;


</function_calls>

<function_calls>

#!/bin/bash

Show the complete section with line numbers for better understanding

wc -l src/sparkwheel/config.py
echo ""
echo "=== Exact section of interest: lines 488-514 ==="
python3 << 'EOF'
with open('src/sparkwheel/config.py', 'r') as f:
lines = f.readlines()
for i in range(487, 514): # 0-indexed, so 487 = line 488
print(f"{i+1}: {lines[i]}", end='')
EOF


</function_calls>

<function_calls>

#!/bin/bash

Now search for build_missing_key_error definition

python3 << 'EOF'
import os
import re

Search all Python files in src/

for root, dirs, files in os.walk('src/'):
for file in files:
if file.endswith('.py'):
filepath = os.path.join(root, file)
with open(filepath, 'r') as f:
content = f.read()
if 'build_missing_key_error' in content:
print(f"Found in: {filepath}")
# Find the line number and show context
lines = content.split('\n')
for i, line in enumerate(lines, 1):
if 'build_missing_key_error' in line:
print(f" Line {i}: {line}")
EOF


</function_calls>

<function_calls>

#!/bin/bash

Let's just look at the entire delitem method

python3 << 'EOF'
with open('src/sparkwheel/config.py', 'r') as f:
lines = f.readlines()
# Find delitem
in_method = False
indent_level = None
for i, line in enumerate(lines, 1):
if 'def delitem' in line:
in_method = True
indent_level = len(line) - len(line.lstrip())
print(f"Found delitem at line {i}\n")

    if in_method:
        current_indent = len(line) - len(line.lstrip())
        # End of method when we hit a line with same or less indent that's not empty
        if i > (i-1) and current_indent <= indent_level and line.strip() and 'def __delitem__' not in line:
            break
        print(f"{i}: {line}", end='')

EOF


</function_calls>

<function_calls>

#!/bin/bash

Simple approach - search for the function and read more context

rg -n "def delitem" src/sparkwheel/config.py -A 60


</function_calls>

<function_calls>

#!/bin/bash

Now let's specifically check what test coverage exists

Search for any file that might test delete operations

rg -n "del |delitem|Delete|delete.*test|test.*delete" --type=py -C 3 | head -200


</function_calls>

<function_calls>

#!/bin/bash

Let's check if there's any pytest or unittest test file

find . -name "test*.py" -o -name "*test.py" 2>/dev/null | head -10


</function_calls>

<function_calls>

#!/bin/bash

Check the entire directory structure

ls -la


</function_calls>

Based on my examination of the codebase, I can now provide a rewritten review comment. The key findings are:

1. I've confirmed that lines 488-514 in src/sparkwheel/config.py contain the strict delete implementation within the __delitem__ method
2. The implementation includes nested key handling (lines 497-508) that splits on ID_SEP_KEY and attempts to get the parent container
3. However, I was unable to locate any test files in the repository to verify the test coverage claim definitively

Let me verify one more thing about the actual implementation:
<function_calls>

#!/bin/bash

Let's see the exact imports and verify build_missing_key_error exists

head -30 src/sparkwheel/config.py


</function_calls>

<function_calls>

#!/bin/bash

Search for "raise.*error" patterns in the delete section

sed -n '488,514p' src/sparkwheel/config.py | grep -n "raise|build_missing"


</function_calls>

Based on my verification of the code in src/sparkwheel/config.py lines 488-514, I can now provide the rewritten review comment:

---

Nested key handling is implemented, but test coverage for strict delete behavior is not verified.

The code correctly handles nested keys (lines 497-508) by splitting on ID_SEP_KEY, extracting parent containers, and listing their keys in error messages. The fallback to top-level keys (line 511) covers the case where a parent doesn't exist.

However, without access to the test suite in this review context, I cannot definitively confirm whether this strict delete behavior—the critical breaking change in this PR—is covered by tests. The implementation appears sound, but it needs explicit test coverage.

Confirm test coverage exists for:

• Deleting non-existent top-level keys
• Deleting non-existent nested keys (e.g., "model::missing")
• The parent-doesn't-exist fallback case (line 511)
• Verifying error messages suggest correct available keys for each scenario

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

tests/test_items.py (1)

457-497: Location-based tests correctly cover Item/Component/Expression wiring

Switching to Location and asserting it is threaded into Item, Component, and Expression error paths looks correct and matches the new API. If you find yourself adding more location tests, consider hoisting Location to a module-level import to avoid repeated local imports, but that’s optional.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between 9ec8958 and 827957e.

📒 Files selected for processing (1)

• tests/test_items.py (6 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

**/*.py

⚙️ CodeRabbit configuration file

Review the Python code for quality and correctness. Ensure variable names adhere to PEP8 style guides, are sensible and informative in regards to their function, though permitting simple names for loop and comprehension variables. Ensure routine names are meaningful in regards to their function and use verbs, adjectives, and nouns in a semantically appropriate way. Docstrings should be present for all definition which describe each variable, return value, and raised exception in the appropriate section of the Google-style of docstrings. Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these. Suggest any enhancements for code improving efficiency, maintainability, comprehensibility, and correctness. Ensure new or modified definitions will be covered by existing or new unit tests.

Files:

• tests/test_items.py

🧬 Code graph analysis (1)

tests/test_items.py (2)

src/sparkwheel/utils/exceptions.py (2)

• InstantiationError (137-140)
• Location (22-43)

src/sparkwheel/items.py (3)

• resolve_args (159-185)
• instantiate (28-32)
• instantiate (194-233)

🔇 Additional comments (3)

tests/test_items.py (3)

190-198: InstantiationError expectation is appropriate

Asserting only that InstantiationError is raised keeps this test stable despite the new, more concise message formatting. No change needed.

---

286-295: Ignoring positional args explicitly is fine

Binding to _ and only asserting on kwargs matches the intent of this test (verifying excluded keys) and aligns with the current resolve_args implementation.

---

498-538: Re-raise semantics for InstantiationError with location are well tested

This test tightly validates the Component.instantiate() branch where an InstantiationError already has source_location: patching sparkwheel.items.instantiate and asserting error is inner_error plus message/suggestion/location fields gives strong regression coverage for the new behavior. Nicely done.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

docs/user-guide/references.md (1)

45-53: Use indented code blocks instead of fenced blocks.

The markdown linter prefers indented code blocks for consistency with the project's style guide. Apply this diff:

-```yaml
-# base.yaml
-vars:
-  features_path: null  # Default, will be overridden
-
-# model.yaml
-dataset:
-  path: "%vars::features_path"  # Local ref - sees CLI override
-```
+    # base.yaml
+    vars:
+      features_path: null  # Default, will be overridden
+    
+    # model.yaml
+    dataset:
+      path: "%vars::features_path"  # Local ref - sees CLI override

-```python
-config = Config()
-config.update("base.yaml")
-config.update("model.yaml")
-config.update("vars::features_path=/data/features.npz")  # CLI override
-
-# Local % ref sees the override!
-path = config.resolve("dataset::path")  # "/data/features.npz"
-```
+    config = Config()
+    config.update("base.yaml")
+    config.update("model.yaml")
+    config.update("vars::features_path=/data/features.npz")  # CLI override
+    
+    # Local % ref sees the override!
+    path = config.resolve("dataset::path")  # "/data/features.npz"

Also applies to: 55-63

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between 827957e and 4b719b7.

📒 Files selected for processing (5)

• docs/user-guide/references.md (3 hunks)
• src/sparkwheel/config.py (17 hunks)
• src/sparkwheel/preprocessor.py (9 hunks)
• tests/test_config.py (5 hunks)
• tests/test_preprocessor.py (2 hunks)

🧰 Additional context used

📓 Path-based instructions (2)

**/*.py

⚙️ CodeRabbit configuration file

Review the Python code for quality and correctness. Ensure variable names adhere to PEP8 style guides, are sensible and informative in regards to their function, though permitting simple names for loop and comprehension variables. Ensure routine names are meaningful in regards to their function and use verbs, adjectives, and nouns in a semantically appropriate way. Docstrings should be present for all definition which describe each variable, return value, and raised exception in the appropriate section of the Google-style of docstrings. Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these. Suggest any enhancements for code improving efficiency, maintainability, comprehensibility, and correctness. Ensure new or modified definitions will be covered by existing or new unit tests.

Files:

• src/sparkwheel/config.py
• src/sparkwheel/preprocessor.py
• tests/test_preprocessor.py
• tests/test_config.py

**/*.md

⚙️ CodeRabbit configuration file

Remember that documentation must be updated with the latest information.

Files:

• docs/user-guide/references.md

🧬 Code graph analysis (3)

src/sparkwheel/config.py (5)

src/sparkwheel/locations.py (3)

• LocationRegistry (8-74)
• merge (50-56)
• get (39-48)

src/sparkwheel/operators.py (2)

• MergeContext (18-94)
• apply_operators (186-365)

src/sparkwheel/path_utils.py (1)

• get_by_id (308-366)

src/sparkwheel/preprocessor.py (3)

• Preprocessor (21-353)
• process_raw_refs (76-117)
• process (119-137)

src/sparkwheel/utils/exceptions.py (1)

• ConfigKeyError (143-198)

src/sparkwheel/preprocessor.py (3)

src/sparkwheel/path_utils.py (4)

• get_by_id (308-366)
• resolve_relative_ids (369-432)
• split_file_and_id (135-164)
• split_file_and_id (219-232)

src/sparkwheel/utils/exceptions.py (2)

• CircularReferenceError (131-134)
• ConfigKeyError (143-198)

src/sparkwheel/locations.py (1)

• LocationRegistry (8-74)

tests/test_config.py (3)

src/sparkwheel/config.py (4)

• Config (115-785)
• update (378-463)
• get (208-263)
• resolve (577-668)

src/sparkwheel/utils/exceptions.py (2)

• ConfigKeyError (143-198)
• ConfigMergeError (201-210)

src/sparkwheel/operators.py (1)

• apply_operators (186-365)

🪛 GitHub Check: codecov/patch

src/sparkwheel/config.py

[warning] 516-516: src/sparkwheel/config.py#L516
Added line #L516 was not covered by tests

src/sparkwheel/preprocessor.py

[warning] 183-183: src/sparkwheel/preprocessor.py#L183
Added line #L183 was not covered by tests

🪛 markdownlint-cli2 (0.18.1)

docs/user-guide/references.md

45-45: Code block style
Expected: indented; Actual: fenced

(MD046, code-block-style)

---

55-55: Code block style
Expected: indented; Actual: fenced

(MD046, code-block-style)

🔇 Additional comments (5)

docs/user-guide/references.md (2)

21-96: Two-phase processing model clearly documented.

The new Eager/Lazy distinction for external vs. local raw references is well explained and aligns with the PR objectives. The resolution flow steps accurately reflect how CLI overrides interact with local refs.

---

238-263: Local raw reference lazy expansion and CLI override behavior well articulated.

The examples and explanations correctly capture how local % refs see CLI overrides while external % refs remain frozen. This is a critical behavioral change introduced in the PR and is documented clearly.

src/sparkwheel/preprocessor.py (2)

269-273: Two-phase expansion correctly implemented.

Lines 269-273 properly implement the Phase 1 logic: when external_only=True and the reference is local (no file path), the raw reference string is returned unchanged for later expansion. This enables CLI overrides to affect values used by local raw references.

---

302-320: Location-aware error handling is thorough.

The error handling correctly:

• Retrieves location info from the registry (lines 306-308)
• Builds context-aware error messages (lines 311-314)
• Wraps exceptions in ConfigKeyError with source location (lines 317-320)

The get_by_id helper already provides detailed error messages with available keys, which are preserved through the wrapping.

src/sparkwheel/config.py (1)

488-519: Based on my verification of the codebase:

Test coverage for line 516 is confirmed; fallback logic properly tested through test_delete_nested_key_when_parent_doesnt_exist.

Line 516 (available_keys = list(self._data.keys()) if isinstance(self._data, dict) else []) executes in the fallback path when attempting to delete a nested key with a non-existent parent. The test by this exact name at line 1020 in tests/test_config.py is specifically designed to cover this scenario.

Supporting evidence from related tests:

• test_delete_nonexistent_top_level_key_shows_available_keys (line 987) — validates available keys in error messages
• test_delete_nonexistent_nested_key_shows_parent_keys (line 1002) — validates parent context in errors

The implementation correctly handles the code path:

• Nested key detection (line 501)
• Parent retrieval attempt (lines 507-512)
• Exception handling fallback (lines 513-516)
• Error reporting with context (line 518)

Line 516 is reachable and covered.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (3)

tests/test_preprocessor.py (3)

23-27: Circular reference tests correctly exercise error type and location tracking

Both circular-reference tests now:

• Use Loader.load_file(... ) and pass locations into process_raw_refs, so location data is exercised.
• Assert CircularReferenceError is raised and, in the second test, that source_location.filepath and the reference-chain details are present.

This is a solid check of both detection and reporting. If you want to tighten it further, you could also assert the line number in test_circular_reference_with_location, but that’s purely optional.

Also applies to: 89-106

---

28-48: Missing-key raw-reference tests are thorough; consider avoiding private _original_message

These three tests do a good job covering:

• A simple missing key with location.
• A missing key in an external file, asserting the message includes the external source.
• A nested path where an intermediate key is missing, asserting the path context in the message.

The overall structure and expectations look correct. The only nit is that all three reach into error._original_message, which is a private attribute by naming convention and could make the tests brittle if the error implementation is refactored. If feasible, consider asserting against str(error) or a dedicated public accessor for the “primary” message instead.

Also applies to: 49-72, 73-88

---

164-266: Two-phase external_only tests nicely pin down semantics; minor DRY opportunity

The TestPreprocessorExternalOnly class:

• Clearly verifies phase-1 vs phase-2 behavior for external vs local refs.
• Covers nested local refs and the “external ref inside a locally-referenced template” scenario, which is an important edge case.

The logic and expectations all look consistent with the documented external_only semantics. If you want to reduce repetition, you could factor Loader()/Preprocessor(loader) construction into a small helper or fixture, but that’s a style/DRY tweak, not a correctness issue.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between 4b719b7 and 25b03d2.

📒 Files selected for processing (1)

• tests/test_preprocessor.py (2 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

**/*.py

⚙️ CodeRabbit configuration file

Review the Python code for quality and correctness. Ensure variable names adhere to PEP8 style guides, are sensible and informative in regards to their function, though permitting simple names for loop and comprehension variables. Ensure routine names are meaningful in regards to their function and use verbs, adjectives, and nouns in a semantically appropriate way. Docstrings should be present for all definition which describe each variable, return value, and raised exception in the appropriate section of the Google-style of docstrings. Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these. Suggest any enhancements for code improving efficiency, maintainability, comprehensibility, and correctness. Ensure new or modified definitions will be covered by existing or new unit tests.

Files:

• tests/test_preprocessor.py

🧬 Code graph analysis (1)

tests/test_preprocessor.py (3)

src/sparkwheel/utils/exceptions.py (2)

• CircularReferenceError (131-134)
• ConfigKeyError (143-198)

src/sparkwheel/loader.py (2)

• load_file (136-176)
• Loader (116-241)

src/sparkwheel/preprocessor.py (1)

• process_raw_refs (76-117)

🔇 Additional comments (2)

tests/test_preprocessor.py (2)

7-7: Exception imports aligned with new error model

Importing CircularReferenceError and ConfigKeyError in tests keeps them coupled to the library’s public error types instead of generic exceptions; this is exactly what we want here.

---

107-119: Positive-path raw-reference expansion coverage looks solid

The success-path tests collectively cover:

• Simple local raw ref expansion.
• Nested path expansion.
• External file references.
• Raw refs inside list items.

This gives good confidence that the core expansion behavior works across common container shapes. No issues spotted here.

Also applies to: 120-132, 133-147, 149-162