Repository navigation
SWE Pipeline: fix IPFSDict.load to replace data and apply lint cleanup - #5
Conversation
…ata-instead-of-replacing-it into swe-pipeline/integration
Codecov Report✅ All modified and coverable lines are covered by tests.
🚀 New features to boost your workflow:
|
WouterGlorieux
left a comment
There was a problem hiding this comment.
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()inload()before loading new data, so existing keys are removed rather than merged with new data - Changed exception type from
ValueErrortoTypeErrorwhen a non-string CID is passed toload() - Updated type annotations from
Dict/Optionalto moderndict/str | Nonesyntax
Code modernization (ipfs_dict_chain/CID.py, IPFS.py, IPFSDictChain.py)
- Replaced
typing.Dict,List,Optional,Tuplewith lowercase built-in generics andX | Noneunion syntax - Changed
__eq__/__ne__type annotations fromAnytoobject(more correct for comparison methods) - Added
# noqa: N999and# noqa: BLE001comments for intentional lint suppressions - Refactored
IPFSDictChain.changes()to iterate withitems()instead of indexing - Changed
super(IPFSDictChain, self).__init__()tosuper().__init__()(modern zero-arg form)
Test updates (tests/)
- Added
test_load_replaces_existing_datato verify the clear-before-load behavior - Updated tests to expect
TypeErrorinstead ofValueErrorfor non-string CID - Changed
datetime.now()todatetime.now(timezone.utc)for timezone-aware tests - Added
# noqa: N999and# noqa: B017comments for lint suppressions - Reorganized imports per ruff rules
Documentation (README.md, CHANGELOG.md)
- Added note about
load()replacing existing data - Documented the
TypeErrorchange in CHANGELOG
Issues & Concerns
- 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.
- 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
left a comment
There was a problem hiding this comment.
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: Addedsuper().clear()inload()before populating data from IPFS, so loading replaces rather than merges existing data. Changed non-string CID exception fromValueErrortoTypeError. Also includes type annotation modernizations (dict[str, ...]instead ofDict[str, ...], etc.).tests/test_IPFSDict.py: Updatedtest_loadto expectTypeErrorinstead ofValueError. Addedtest_load_replaces_existing_datawhich 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 theTypeErrorchange.- 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
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: Addedsuper().clear()before populating the dictionary from IPFS data, so loading into an already-populatedIPFSDictno longer merges and leaves stale keys.IPFSDict.load()raisesTypeErrorfor non-string CIDs: Changed fromValueErrortoTypeErrorto better match Python conventions for type validation.IPFSDictChain.changes()key iteration bug: The loop that detects new keys was incorrectly indexingcurrent_items(a dict) with a key from a list-style iteration. It now properly iterates withkey, valueunpacking.Lint and style cleanup
Dict/List/Tuple/Optionalreplaced with builtin generics (dict,list,tuple,X | None).# noqacomments for module naming (N999) and broad exception handling (BLE001).super(IPFSDictChain, self)withsuper().setattr()calls to direct attribute assignment in tests.datetime.now()to timezone-awaredatetime.now(timezone.utc)in tests.Documentation
CHANGELOG.mdto document the newload()replace behavior and theTypeErrorchange.README.mdwith a note clarifying thatIPFSDict.load()replaces existing data rather than merging.Testing
test_load_replaces_existing_dataintests/test_IPFSDict.pyto verify that loading into a populatedIPFSDictclears old keys and loads only the new data.test_loadintests/test_IPFSDict.pyto expectTypeErrorinstead ofValueErrorfor non-string CIDs.CID,IPFS,IPFSDict, andIPFSDictChaincontinue to pass after the lint cleanup and functional changes.unittestand pass successfully.