Skip to content

SWE Pipeline: Automated code improvements for ipfs_dict_chain - #7

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

WouterGlorieux merged 10 commits into
mainfrom
swe-pipeline/integration

Conversation

@WouterGlorieux

Copy link
Copy Markdown
Contributor

SWE Pipeline — Automated Code Improvements

This pull request was automatically generated by the SWE Pipeline.

Summary

  • 1 issues fixed: 1 bugs, 0 refactors, 0 enhancements
  • Coverage: 100%
  • Tests: all passing

What changed

See the commit history on the swe-pipeline/integration branch for the full list of changes.

Review

The pipeline performed multiple code reviews and all issues found were addressed. The codebase is at 100% test coverage with a green test suite.

Automatically generated at commit 0a3abec2.

@codecov

codecov Bot commented Sep 9, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

Files with missing lines Coverage Δ
ipfs_dict_chain/IPFSDict.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 fixes a significant API-breaking bug where dictionary data keys could shadow IPFSDict class methods (like save(), load(), and cid()) when accessed via dot notation. The fix inverts the attribute lookup logic in __getattribute__ — instead of checking data keys first (with a hardcoded exclusion list for dict methods), it now checks class attributes/methods first and falls back to data keys only when no matching class attribute exists. This is a more general and robust approach that protects all class methods, not just the ones in the previously hardcoded _DICT_METHODS set.

The PR includes comprehensive test coverage for the new behavior, documentation updates in both the README and CHANGELOG, and a couple of minor style cleanups from ruff auto-fixes. All 142 tests pass with 100% coverage.

Changes Overview

Core fix — ipfs_dict_chain/IPFSDict.py:

  • Removed the _DICT_METHODS frozenset that hardcoded a list of protected dict methods.
  • Rewrote __getattribute__ to try super().__getattribute__(key) first (which finds class methods and attributes), then fall back to super().__getitem__(key) for data keys, and finally raise AttributeError for genuinely missing attributes.

Tests — tests/test_IPFSDict.py:

  • Added 7 new tests covering: method names not shadowing methods (save, load, cid), method calls still working when a colliding data key exists, regular data keys still accessible via dot notation, attribute assignment to method names, bracket-notation access to colliding keys, and missing attributes raising AttributeError.

Style — ipfs_dict_chain/IPFS.py and tests/test_IPFS.py:

  • Removed an unnecessary # noqa: BLE001 comment from the connect() method's except block (ruff auto-fix).
  • Removed a stray blank line in test imports.

Documentation — README.md and CHANGELOG.md:

  • Added a note explaining that class methods take precedence over data keys in dot notation, and that bracket notation should be used to access raw data values when a key collides with a method name.
  • Added a CHANGELOG entry describing the fix.

Issues & Concerns

  1. The new test_nonexistent_attribute_raises test in tests/test_IPFSDict.py is redundant with the existing test_error_handling test, which already verifies that accessing a non-existent attribute raises AttributeError. While having a dedicated test is not harmful, it does add slight duplication. Consider consolidating or removing the redundant assertion to keep the test suite focused.

  2. The ruff auto-fix removed the # noqa: BLE001 comment from the except Exception block in connect() (in ipfs_dict_chain/IPFS.py), but the other except Exception blocks in the same file (_add_json, _get_json, _test_connection) still carry the comment. Since ruff passes without flagging any of these, the remaining # noqa: BLE001 comments on the other blocks are also unnecessary. This is a minor consistency issue — either remove the noqa comments from all blocks or keep them on all blocks for uniformity.

  3. The new __getattribute__ implementation relies on a try/except AttributeError for every data-key access via dot notation. This is a known Python anti-pattern that can theoretically mask real AttributeError exceptions raised from within property getters or descriptors. In the current codebase this is not a practical concern since IPFSDict and its parents define no properties or descriptors, but it's worth noting as a design trade-off. The alternative of checking type(self).__dict__ or using a precomputed set of method names would avoid the exception-based flow control, though the current approach is more maintainable since it automatically covers future methods added to the class.

Recommendation

Changes needed before merging — the fix itself is correct, well-tested, and properly documented, but the redundant test in item 1 should be cleaned up to avoid duplication in the test suite. Items 2 and 3 are minor and non-blocking but worth addressing for code quality consistency.

@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 fixes a critical API issue where dictionary data keys could shadow IPFSDict class methods (save(), load(), cid()) when accessed via dot notation, breaking the class's public API. The fix replaces the hardcoded _DICT_METHODS frozenset with a dynamic MRO-based check in __getattribute__, ensuring that any real class attribute or method (from any class in the inheritance chain) takes precedence over data keys during attribute-style access. The change also simplifies the fallback path so that accessing a genuinely missing attribute raises AttributeError directly rather than falling through to super().__getattribute__.

The implementation is clean, well-tested, and properly documented. All 141 tests pass with 100% coverage, and the new tests comprehensively cover the fix's behavior: method names remain callable when shadowed by data keys, regular data keys still work via dot notation, and data remains accessible via bracket notation when key names collide with methods.

Changes Overview

  • ipfs_dict_chain/IPFSDict.py: Removed the hardcoded _DICT_METHODS frozenset. Rewrote __getattribute__ to check the MRO (any(key in cls.__dict__ for cls in type(self).__mro__)) to determine if a key refers to a real class attribute/method before falling back to dict data lookup. Changed the missing-key fallback to raise AttributeError(key) directly.
  • tests/test_IPFSDict.py: Added 5 new tests covering method-name shadowing scenarios (save/load/cid), method callability with shadowed keys, regular data key access, attribute assignment to method names, and bracket-notation access to shadowed keys. Removed one redundant test (test_nonexistent_attribute_raises) that duplicated coverage already present in test_error_handling.
  • tests/test_IPFS.py: Removed a blank line (ruff auto-fix).
  • README.md: Added a note explaining that class methods take precedence over data keys in dot notation, and that bracket notation should be used to access raw data when a key collides with a method name.
  • CHANGELOG.md: Documented the fix.

Issues & Concerns

No issues identified.

The fix is correct and addresses the reported problem thoroughly. The MRO-based approach is more robust than the previous hardcoded method-name list — it automatically covers all methods from all classes in the inheritance chain (including IPFSDict's own save, load, cid, and any future methods added to the class or its parents). The behavior is consistent: data keys that collide with method names remain stored and accessible via bracket notation (d['save']), while dot notation (d.save) always returns the bound method. The AttributeError fallback for genuinely missing attributes is preserved. Subclasses like IPFSDictChain are also correctly protected since the MRO check traverses the full inheritance chain.

Recommendation

Ready to merge

@WouterGlorieux
WouterGlorieux merged commit 6aee04c into main Sep 9, 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