Skip to content

SWE Pipeline: fix IPFSDict.load to replace data and apply lint cleanup - #5

Merged
WouterGlorieux merged 9 commits into
mainfrom
swe-pipeline/integration
Sep 8, 2026
Merged

WouterGlorieux merged 9 commits into
mainfrom
swe-pipeline/integration

Conversation

@WouterGlorieux

Copy link
Copy Markdown
Contributor

Summary

This PR fixes a critical bug in IPFSDict.load() where loading new data into an already-populated dictionary merged the new data with existing data, leaving stale keys behind. The method now clears all existing data before loading, ensuring the dictionary fully reflects the state stored at the given CID.

The PR also includes a related fix in IPFSDictChain.changes() that resolves incorrect handling of dictionary items during change detection, plus a broad lint/style cleanup across the codebase.

Changes

Functional fixes

  • IPFSDict.load() now replaces existing data: Added super().clear() before populating the dictionary from IPFS data, so loading into an already-populated IPFSDict no longer merges and leaves stale keys.
  • IPFSDict.load() raises TypeError for non-string CIDs: Changed from ValueError to TypeError to better match Python conventions for type validation.
  • Fixed IPFSDictChain.changes() key iteration bug: The loop that detects new keys was incorrectly indexing current_items (a dict) with a key from a list-style iteration. It now properly iterates with key, value unpacking.

Lint and style cleanup

  • Applied ruff safe auto-fixes (52 changes) across all source and test files.
  • Fixed remaining ruff errors, including:
    • Modernized type hints: Dict/List/Tuple/Optional replaced with builtin generics (dict, list, tuple, X | None).
    • Added # noqa comments for module naming (N999) and broad exception handling (BLE001).
    • Removed unused imports and reordered imports per style rules.
    • Replaced super(IPFSDictChain, self) with super().
    • Changed setattr() calls to direct attribute assignment in tests.
    • Updated datetime.now() to timezone-aware datetime.now(timezone.utc) in tests.

Documentation

  • Updated CHANGELOG.md to document the new load() replace behavior and the TypeError change.
  • Updated README.md with a note clarifying that IPFSDict.load() replaces existing data rather than merging.

Testing

  • Added test_load_replaces_existing_data in tests/test_IPFSDict.py to verify that loading into a populated IPFSDict clears old keys and loads only the new data.
  • Updated test_load in tests/test_IPFSDict.py to expect TypeError instead of ValueError for non-string CIDs.
  • Existing test suites for CID, IPFS, IPFSDict, and IPFSDictChain continue to pass after the lint cleanup and functional changes.
  • All tests were run with unittest and pass successfully.

@codecov

codecov Bot commented Sep 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

Files with missing lines Coverage Δ
ipfs_dict_chain/CID.py 100.00% <100.00%> (ø)
ipfs_dict_chain/IPFS.py 100.00% <100.00%> (ø)
ipfs_dict_chain/IPFSDict.py 100.00% <100.00%> (ø)
ipfs_dict_chain/IPFSDictChain.py 100.00% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@WouterGlorieux WouterGlorieux left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

This PR addresses a real bug where IPFSDict.load() merged new data with existing data instead of replacing it, leaving stale keys behind. The fix adds super().clear() before loading new data in load(), ensuring a clean replacement. The PR also includes a substantial amount of code modernization: replacing typing.Dict/List/Optional/Tuple with modern lowercase built-in generics and X | None syntax, changing ValueError to the more semantically correct TypeError for non-string CID arguments, adding # noqa comments for intentional lint suppressions, and cleaning up test code style via ruff auto-fixes.

Overall, the changes are well-structured and the core fix is correct. All 130 tests pass with 100% coverage. The lint and style commits are reasonable modernization efforts that don't change runtime behavior. The documentation (README and CHANGELOG) has been updated to reflect the behavior change. However, there are a couple of minor issues worth addressing before merge.

Changes Overview

Core fix (ipfs_dict_chain/IPFSDict.py)

  • Added super().clear() in load() before loading new data, so existing keys are removed rather than merged with new data
  • Changed exception type from ValueError to TypeError when a non-string CID is passed to load()
  • Updated type annotations from Dict/Optional to modern dict/str | None syntax

Code modernization (ipfs_dict_chain/CID.py, IPFS.py, IPFSDictChain.py)

  • Replaced typing.Dict, List, Optional, Tuple with lowercase built-in generics and X | None union syntax
  • Changed __eq__/__ne__ type annotations from Any to object (more correct for comparison methods)
  • Added # noqa: N999 and # noqa: BLE001 comments for intentional lint suppressions
  • Refactored IPFSDictChain.changes() to iterate with items() instead of indexing
  • Changed super(IPFSDictChain, self).__init__() to super().__init__() (modern zero-arg form)

Test updates (tests/)

  • Added test_load_replaces_existing_data to verify the clear-before-load behavior
  • Updated tests to expect TypeError instead of ValueError for non-string CID
  • Changed datetime.now() to datetime.now(timezone.utc) for timezone-aware tests
  • Added # noqa: N999 and # noqa: B017 comments for lint suppressions
  • Reorganized imports per ruff rules

Documentation (README.md, CHANGELOG.md)

  • Added note about load() replacing existing data
  • Documented the TypeError change in CHANGELOG

Issues & Concerns

  1. Docstring inconsistency in IPFSDict.load()

The load() method's docstring at line 80 of ipfs_dict_chain/IPFSDict.py still says :raises ValueError: If the CID is not a string, but the code now raises TypeError (changed in commit 9d006e8). This documentation is now inaccurate and could mislead users about the exception type they should catch. The docstring should be updated to say :raises TypeError: to match the actual behavior.

  1. Minor test coverage gap in test_load_replaces_existing_data

The new test in tests/test_IPFSDict.py sets up a shared_key in the initial dict but never verifies it's removed after load(). The test name implies it verifies full replacement behavior, but it only checks that old_key is gone and new_key is present. Since shared_key exists in the old data but not in the new data being loaded, the test should also assert self.assertNotIn('shared_key', ipfs_dict) to fully verify the replacement semantics. This is a minor gap, not a correctness issue with the fix itself.

Recommendation

Changes needed before merging. The core fix is correct and well-implemented, but the docstring inconsistency in IPFSDict.load() should be corrected (it still documents ValueError while the code raises TypeError), and the new test could be strengthened by verifying the removal of the shared_key it sets up. These are minor issues that don't affect the correctness of the fix, but they should be addressed for code quality and accuracy.

@WouterGlorieux WouterGlorieux left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

This PR addresses the issue where IPFSDict.load() merged new data with existing data instead of replacing it. The fix adds a super().clear() call in load() before populating the dict from IPFS, ensuring that loading into an already-populated IPFSDict fully replaces the existing contents. The PR also changes the exception type for non-string CIDs from ValueError to TypeError (semantically more appropriate), updates documentation (CHANGELOG, README), and includes comprehensive test coverage for the new behavior.

This is the second review round. The revision fixes from round 1 (changing ValueError to TypeError) and round 2 (adding the shared_key assertion to the test) are correct and address the previously identified issues. All 130 tests pass and linting is clean.

Changes Overview

  • ipfs_dict_chain/IPFSDict.py: Added super().clear() in load() before populating data from IPFS, so loading replaces rather than merges existing data. Changed non-string CID exception from ValueError to TypeError. Also includes type annotation modernizations (dict[str, ...] instead of Dict[str, ...], etc.).
  • tests/test_IPFSDict.py: Updated test_load to expect TypeError instead of ValueError. Added test_load_replaces_existing_data which verifies old keys are removed, shared keys are replaced, and new keys are added when loading into a populated dict.
  • CHANGELOG.md / README.md: Documented the new replace-on-load behavior and the TypeError change.
  • Other files (CID.py, IPFS.py, IPFSDictChain.py, test files): Lint/type-annotation cleanups (ruff fixes, import reordering, datetime.now(timezone.utc) for timezone-aware datetimes, etc.). These are mechanical and behavior-preserving.

Issues & Concerns

No issues identified.

The core change is correct and well-tested. The clear() call is placed after the get_json() call and the isinstance(data, dict) check, so if IPFS retrieval fails or the data is not a dict, the existing data is preserved (no partial state corruption). The new test test_load_replaces_existing_data properly covers the three key scenarios: old keys removed, shared keys replaced, and new keys added. The exception type change from ValueError to TypeError is semantically correct since passing a non-string where a string is expected is a type error. Documentation updates accurately describe the new behavior.

Recommendation

Ready to merge

@WouterGlorieux
WouterGlorieux merged commit ec74e88 into main Sep 8, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant