Fix custom dict overriding in collections
#722
Conversation
WalkthroughThe changes introduce a new Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #722 +/- ##
==========================================
+ Coverage 82.57% 84.12% +1.54%
==========================================
Files 27 27
Lines 1584 1625 +41
Branches 284 296 +12
==========================================
+ Hits 1308 1367 +59
+ Misses 215 196 -19
- Partials 61 62 +1 ☔ View full report in Codecov by Sentry. |
DanielYang59
changed the title
monty.collections
DanielYang59
changed the title
monty.collectionscollections
src/monty/collections.py
Outdated
| >>> t = dict2namedtuple([("foo", 1), ("bar", "hello")]) | ||
| >>> assert t[0] == t.foo and t[1] == t.bar | ||
| >>> tpl = dict2namedtuple([("foo", 1), ("bar", "hello")]) | ||
| >>> assert tpl[0] is tpl.foo and t[1] is tpl.bar |
There was a problem hiding this comment.
I believe we should check identity (is) instead of value equality (==), as both should point to the same object.
DanielYang59
changed the title
collectionscollections
DanielYang59
changed the title
collectionscollections
DanielYang59
force-pushed
the
case-insensitive-dict
branch
from
7fdd1d7
to
a356b12
Compare
|
|
||
| class Namespace(dict): |
There was a problem hiding this comment.
If I understand correctly, Namespace is a dict that only allows adding new items but rejects updating existing items, however I don't really understand why it's called Namespace?
monty/src/monty/collections.py
Lines 57 to 58 in a3d35a6
The original implementation also allows "delete" item and current implementation would NOT allow such operation:
- To agree with the docstring that "does not permit to redefine its keys" (apparently delete would alter existing keys)
- Prevent "hacky" way to change values by "delete and add back" (though the code is designed to protect against such attempt to modify its value, as the mutability switch is not truly "private" in Python)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
tests/test_collections.py (2)
19-25: Consider adding more comprehensive test casesWhile the basic functionality is tested, consider adding these test cases for better coverage:
- Error cases (e.g., accessing non-existent deep paths)
- Deep copy verification to ensure nested dictionaries are independent
- Cycle detection in nested structures
Example additional test cases:
def test_tree(): x = tree() x["a"]["b"]["c"]["d"] = 1 # Existing tests... # Test error cases with pytest.raises(KeyError): _ = x["nonexistent"]["path"] # Test deep copy y = x["a"] y["new"] = "value" assert "new" not in x["a"] # Test nested references z = tree() z["self"] = z assert z["self"] is z
28-124: Excellent test coverage with room for additional edge casesThe test suite thoroughly covers the core functionality of ControlledDict with different permission combinations. However, consider adding these edge cases:
def test_controlled_dict_edge_cases(): # Test nested dictionary behavior dct = ControlledDict({"nested": {"a": 1}}) dct._allow_update = False with pytest.raises(TypeError, match="update is disabled"): dct["nested"]["a"] = 2 # Test copy behavior import copy dct = ControlledDict({"a": 1}) dct._allow_update = False copied = copy.copy(dct) assert copied._allow_update == False deep_copied = copy.deepcopy(dct) assert deep_copied._allow_update == Falsesrc/monty/collections.py (2)
7-11: Correct grammatical errors in class descriptionsThere are minor grammatical errors in the class descriptions.
Apply the following diff to correct them:
- - Namespace: A dict doesn't allow changing values, but could - add new keys, + - Namespace: A dict that doesn't allow changing values but can + add new keys. - - AttrDict: A dict whose values could be access as `dct.key`. + - AttrDict: A dict whose values can be accessed as `dct.key`. - - MongoDict: A dict-like object whose values are nested dicts - could be accessed as attributes. + - MongoDict: A dict-like object whose nested dict values can be + accessed as attributes.
309-309: Typo in variable name in example codeThere is a typo in the assertion:
t[1]should betpl[1].Apply the following diff to correct it:
->>> assert tpl[0] is tpl.foo and t[1] is tpl.bar +>>> assert tpl[0] is tpl.foo and tpl[1] is tpl.bar
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
src/monty/collections.py(4 hunks)src/monty/design_patterns.py(2 hunks)src/monty/fractions.py(1 hunks)src/monty/functools.py(1 hunks)tests/test_collections.py(1 hunks)
🔇 Additional comments (8)
src/monty/fractions.py (1)
8-11: LGTM! Good optimization of type hints.
The conditional import of Sequence under TYPE_CHECKING is a good practice as it:
- Reduces runtime overhead by importing type hints only when needed
- Maintains static type checking capabilities without impacting performance
Let's verify this pattern is consistently applied across the codebase:
✅ Verification successful
Verified: Type hint optimization is correctly implemented
The Sequence type is only used as a type hint in the gcd_float function's signature and is not used in runtime operations. The conditional import under TYPE_CHECKING is appropriate here. Additionally, this pattern is consistently applied across the codebase, as evidenced by the widespread use of TYPE_CHECKING guards in multiple modules.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other files that might benefit from similar optimization
# Look for direct imports of Sequence that could be moved under TYPE_CHECKING
# Find files directly importing Sequence
rg "from typing import .*Sequence" --type py
# Find files already using TYPE_CHECKING pattern
rg "if TYPE_CHECKING:" --type py
Length of output: 994
Script:
#!/bin/bash
# Let's check the usage of Sequence type in fractions.py to ensure it's only used for type hints
ast-grep --pattern 'def $FUNC($$$) -> $$$:
$$$' src/monty/fractions.py
# Also check for any runtime usage of Sequence
rg "Sequence\[" src/monty/fractions.py
Length of output: 2995
src/monty/design_patterns.py (3)
10-10: LGTM! Good type hinting optimization.
The conditional import of Any under TYPE_CHECKING is a recommended pattern that improves type checking without runtime overhead.
Also applies to: 13-14
101-101: LGTM! Good type hint modernization.
The update to use lowercase type hints (tuple, dict instead of Tuple, Dict) aligns with PEP 585 and modern Python type hinting practices.
Line range hint 39-116: Verify cached_class compatibility with new dictionary types.
Since the PR introduces new controlled dictionary types, we should verify that the cached_class decorator works correctly with them, especially for caching and pickling behavior.
tests/test_collections.py (2)
3-16: LGTM! Clean import organization
The imports are well-organized and include all necessary components for testing the collections module.
126-150: LGTM! Proper error type for immutability violations
The tests correctly validate that frozendict now raises TypeError instead of KeyError when attempting modifications, which aligns with the PR objectives and Python's standard behavior for immutable types.
src/monty/functools.py (2)
13-13: LGTM! Good type hint organization.
Moving type hints under TYPE_CHECKING is a good practice as it optimizes runtime performance by excluding type-checking imports during execution.
Also applies to: 15-16
13-13: Verify the removal of namedtuple import.
The AI summary mentions the removal of namedtuple import and _CacheInfo. Let's verify if this removal has any implications.
Also applies to: 15-16
✅ Verification successful
The removal of namedtuple import is safe
The namedtuple import was only used by _CacheInfo which has been removed. The only remaining usage of namedtuple is in collections.py where it's properly imported from the standard library's collections module. The removal of the import from functools.py doesn't affect any functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining usage of namedtuple or _CacheInfo
# Search for namedtuple usage
echo "Searching for namedtuple usage..."
rg "namedtuple|_CacheInfo" -A 5
# Search for cache info related patterns
echo "Searching for cache info related patterns..."
rg "cache_info|_cache_info" -A 5
# Look for any LRU cache implementations that might use CacheInfo
echo "Searching for LRU cache implementations..."
ast-grep --pattern 'class $_ {
$$$
cache_info($$$) {
$$$
}
$$$
}'
Length of output: 4909
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/monty/collections.py (2)
66-68: Add type hints for mutability flags.The class attributes controlling mutability should have type hints for better code maintainability.
- _allow_add: bool = True - _allow_del: bool = True - _allow_update: bool = True + _allow_add: bool = True + _allow_del: bool = True + _allow_update: bool = True + """Control flags for dictionary mutability."""
81-88: Make error messages more consistent.The error messages across different methods have varying formats. Consider standardizing them for better user experience.
- raise TypeError(f"Cannot add new key {key!r}, because add is disabled.") + raise TypeError(f"{self.__class__.__name__} does not support adding new key {key!r}") - raise TypeError(f"Cannot update key {key!r}, because update is disabled.") + raise TypeError(f"{self.__class__.__name__} does not support updating key {key!r}")Also applies to: 90-102, 104-120, 123-145
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/monty/collections.py(4 hunks)
🧰 Additional context used
📓 Learnings (1)
src/monty/collections.py (1)
Learnt from: DanielYang59
PR: materialsvirtuallab/monty#722
File: src/monty/collections.py:217-220
Timestamp: 2024-11-27T11:23:01.603Z
Learning: Using `super().__getattribute__` in `FrozenAttrDict.__getattribute__` can cause a RecursionError due to interaction with attribute access in `__setattr__`. To prevent recursion, use `object.__getattribute__` instead.
🔇 Additional comments (4)
src/monty/collections.py (4)
2-12: LGTM! Well-structured module documentation.
The module docstring clearly describes each collection class and its purpose. The new imports are appropriate for the implementation.
Also applies to: 18-19
148-164: LGTM! Clean implementation of specialized dictionary types.
The frozendict and Namespace classes effectively use ControlledDict to implement their specific behaviors.
189-197: LGTM! Improved attribute handling and warnings.
The implementation correctly:
- Warns about shadowed dict methods
- Uses
object.__getattribute__to prevent recursion issues
Also applies to: 214-228
240-244: LGTM! Enhanced documentation with clear examples.
The added examples and warnings in the documentation provide clear guidance for users.
Also applies to: 302-317
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/monty/collections.py(4 hunks)
🧰 Additional context used
📓 Learnings (1)
src/monty/collections.py (1)
Learnt from: DanielYang59
PR: materialsvirtuallab/monty#722
File: src/monty/collections.py:217-220
Timestamp: 2024-11-27T11:23:01.603Z
Learning: Using `super().__getattribute__` in `FrozenAttrDict.__getattribute__` can cause a RecursionError due to interaction with attribute access in `__setattr__`. To prevent recursion, use `object.__getattribute__` instead.
🔇 Additional comments (6)
src/monty/collections.py (6)
2-12: LGTM! Well-structured module documentation.
The module docstring clearly describes all collection classes and their purposes.
41-68: LGTM! Well-designed base class for controlled mutability.
The ControlledDict class provides a robust foundation for implementing dictionaries with configurable mutability. The implementation:
- Uses clear flag names for controlling operations
- Provides comprehensive docstrings with references
- Follows the Single Responsibility Principle
148-164: LGTM! Clear and consistent implementation of specialized dictionaries.
Both classes effectively use ControlledDict to implement their specific behaviors:
frozendict: Completely immutable (all operations disabled)Namespace: Semi-mutable (only additions allowed)
189-197: LGTM! Good addition of warning for shadowed dict methods.
The warning helps prevent subtle bugs when attribute names conflict with dictionary methods.
214-218: LGTM! Correctly fixed the recursion issue.
The implementation now uses object.__getattribute__ to prevent recursion, as discussed in the previous review.
104-104:
Fix return type hint in setdefault method.
The method's return type hint should be Any instead of None as it returns the value associated with the key.
- def setdefault(self, key, default=None) -> None:
+ def setdefault(self, key, default=None) -> Any:
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
tests/test_collections.py (3)
19-25: Enhance test coverage for edge cases in tree()Consider adding test cases for:
- Accessing non-existent deep paths
- Default value behavior
- Empty path handling
def test_tree(): x = tree() x["a"]["b"]["c"]["d"] = 1 # Test existing assertions... # Test non-existent deep path assert isinstance(x["new"]["path"], dict) # Test default value y = tree(default=list) assert isinstance(y["new"]["path"], list) # Test empty path assert isinstance(x[""], dict)
28-124: Add test coverage for additional dictionary methodsThe test suite should include coverage for other dictionary methods like:
copy()get()items(),keys(),values()def test_dict_methods(self): dct = ControlledDict(a=1, b=2) # Test copy copy_dct = dct.copy() assert copy_dct == dct assert copy_dct is not dct # Test get assert dct.get('a') == 1 assert dct.get('missing', 'default') == 'default' # Test views assert list(dct.keys()) == ['a', 'b'] assert list(dct.values()) == [1, 2] assert list(dct.items()) == [('a', 1), ('b', 2)]
251-262: Enhance dict2namedtuple test coverageConsider adding test cases for:
- Nested dictionaries
- Invalid field names
- Empty input
- Type conversion edge cases
def test_dict2namedtuple(): # Existing tests... # Test nested dict nested = dict2namedtuple({'outer': {'inner': 'value'}}) assert nested.outer.inner == 'value' # Test invalid field names with pytest.raises(ValueError): dict2namedtuple({'invalid-name': 1}) # Test empty input empty = dict2namedtuple({}) assert len(empty) == 0 # Test type conversion conv = dict2namedtuple({'int_field': '123'}) assert isinstance(conv.int_field, str)src/monty/collections.py (2)
70-78: Consider adding a comment explaining the try-finally pattern.While the implementation is correct, it might be helpful to add a comment explaining why the try-finally pattern is necessary (to ensure
_allow_addis restored even if initialization fails).def __init__(self, *args, **kwargs) -> None: """Temporarily allow add during initialization.""" original_allow_add = self._allow_add + # Use try-finally to ensure _allow_add is restored even if initialization fails try: self._allow_add = True super().__init__(*args, **kwargs) finally: self._allow_add = original_allow_add
104-120: Consider optimizingsetdefaultfor common case.The current implementation always checks
_allow_updateeven when the key exists and the default value matches the current value. This could be optimized for the common case.def setdefault(self, key, default=None) -> Any: if key not in self.data: if not self._allow_add: raise TypeError( f"Cannot add new key using setdefault: {key!r}, because add is disabled." ) elif not self._allow_update: - raise TypeError( - f"Cannot update key using setdefault: {key!r}, because update is disabled." - ) + # Only raise if the default value would actually update the current value + current = self.data[key] + if current != default: + raise TypeError( + f"Cannot update key using setdefault: {key!r}, because update is disabled." + ) + return current return super().setdefault(key, default)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/monty/collections.py(4 hunks)tests/test_collections.py(1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/monty/collections.py (1)
Learnt from: DanielYang59
PR: materialsvirtuallab/monty#722
File: src/monty/collections.py:217-220
Timestamp: 2024-11-27T11:23:01.603Z
Learning: Using `super().__getattribute__` in `FrozenAttrDict.__getattribute__` can cause a RecursionError due to interaction with attribute access in `__setattr__`. To prevent recursion, use `object.__getattribute__` instead.
🔇 Additional comments (7)
tests/test_collections.py (1)
242-249: 🛠️ Refactor suggestion
Expand MongoDict test coverage
The current test coverage for MongoDict is still minimal. This is a duplicate of a previous review comment requesting additional test cases.
src/monty/collections.py (6)
1-19: LGTM! Clear and well-structured module documentation.
The module docstring provides accurate descriptions of all collection classes, and the imports are appropriate for the new functionality.
41-68: LGTM! Well-designed base class with clear documentation.
The ControlledDict class provides a robust foundation for implementing dictionaries with configurable mutability. The docstring is comprehensive and includes helpful references to Python documentation.
148-164: LGTM! Clean implementation of specialized dictionary types.
Both classes effectively use ControlledDict to implement their specific behaviors:
frozendict: Properly immutable (no add/update/delete)Namespace: Allows adding new keys but prevents modifications
The implementation aligns with the PR objectives to improve error handling for frozen dictionaries.
189-197: LGTM! Good addition of warning for method shadowing.
The warning helps prevent subtle bugs when attribute names conflict with dictionary methods.
214-225: LGTM! Correctly implements attribute access using object.getattribute.
The implementation follows the learning from previous reviews to prevent recursion issues.
302-317: LGTM! Clear and informative docstring with examples and warnings.
The docstring effectively communicates the function's behavior, limitations, and potential pitfalls.
Summary
collectionsTypeErrorinstead ofKeyError, in our case, it's "the setter being called on the inappropriate type" instead of "key missing":frozensetit would raise:TypeError: 'frozenset' object does not support item assignment:Custom dicts don't seem to be overridden correctly/fully in
collectionshttps://docs.python.org/3/library/stdtypes.html#mapping-types-dict
frozendictTypeError: Cannot overwrite existing key: keydel/popmethod is not overridden, key could be removedFrozenAttrDictSummary by CodeRabbit
New Features
ControlledDictclass for enhanced control over dictionary operations.ControlledDict,MongoDict, anddict2namedtuple.Bug Fixes
ControlledDict,frozendict, andNamespace.Documentation
Tests