Fix collections.ControlledDict handling of Iterators via update method
#732
Conversation
WalkthroughThe pull request introduces changes to several files, primarily focusing on enhancing Python version support in testing and project configuration. The Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
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 ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #732 +/- ##
=======================================
Coverage 84.19% 84.19%
=======================================
Files 27 27
Lines 1670 1670
Branches 314 313 -1
=======================================
Hits 1406 1406
Misses 202 202
Partials 62 62 ☔ View full report in Codecov by Sentry. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
tests/test_collections.py (1)
65-68: LGTM! Consider adding more iterator test cases.The test for iterator handling is a good addition and aligns with the PR objective. However, consider enhancing test coverage with additional cases:
# Test empty iterator dct.update(zip([], [])) assert len(dct) == previous_len # No changes # Test exhausted iterator iterator = zip(["e"], [13]) list(iterator) # exhaust it dct.update(iterator) assert "e" not in dct # No changes # Test iterator with invalid values with pytest.raises(ValueError): dct.update(zip(["f"], [])) # uneven length.github/workflows/test.yml (1)
12-12: LGTM! Consider matrix optimization.The addition of Python 3.11 and 3.13 to the test matrix improves coverage. However, with 4 Python versions and 2 OS targets, you're now running 8 parallel jobs.
Consider if you need to test all Python versions on all OS targets. You might optimize by:
- Testing all Python versions on Ubuntu
- Testing only LTS Python versions (3.10, 3.12) on macOS
This would reduce the job count while maintaining good coverage.src/monty/collections.py (1)
Line range hint
92-104: LGTM with minor performance optimization suggestion.The changes correctly fix the iterator handling by constructing a dictionary before validation. However, the dict construction is performed twice unnecessarily.
Consider optimizing the validation loop:
- updates = dict(*args, **kwargs) - for key in dict(*args, **kwargs): + updates = dict(*args, **kwargs) + for key in updates: if key not in self.data and not self._allow_add: raise TypeError( f"Cannot add new key {key!r} using update, because add is disabled." ) elif key in self.data and not self._allow_update: raise TypeError( f"Cannot update key {key!r} using update, because update is disabled." ) super().update(updates)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
.github/workflows/test.yml(1 hunks)pyproject.toml(1 hunks)src/monty/collections.py(2 hunks)tests/test_collections.py(2 hunks)
🔇 Additional comments (2)
tests/test_collections.py (1)
164-167: LGTM! Clear test case for Namespace update behavior.
The test case clearly documents that update can be used to add new values to a Namespace, which is distinct from updating existing values.
src/monty/collections.py (1)
Line range hint 92-104: Verify fix against pymatgen issue.
Let's verify that this change resolves the iterator handling issue mentioned in pymatgen PR #4223.
✅ Verification successful
Iterator handling in ControlledDict.update() is properly tested and working
Looking at the test coverage in tests/test_collections.py, specifically the test_update_allowed method, there is explicit testing for iterator handling:
# Test Iterator handling
dct.update(zip(["c", "d"], [11, 12]))
assert dct["c"] == 11The implementation in the review correctly handles iterators by using dict(*args, **kwargs) to process the input before updating, and the test suite verifies this behavior. The changes appear to properly resolve any iterator-related issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for related test cases and usage patterns in pymatgen
# Look for ControlledDict usage with iterators in tests
rg -A 5 "ControlledDict.*update.*zip\(" || \
rg -A 5 "ControlledDict.*update.*iter\(" || \
rg -A 5 "ControlledDict.*update.*map\("
# Look for any failing tests related to ControlledDict
rg -B 2 -A 5 "def test.*ControlledDict.*fail"
Length of output: 174
Script:
#!/bin/bash
# Let's try a broader search to understand the context and test coverage
# Search for ControlledDict tests
rg -B 2 -A 5 "class.*Test.*ControlledDict"
# Look for update method tests specifically
rg -B 2 -A 5 "test.*update" src/monty/collections.py
# Search for any test files that might contain ControlledDict tests
fd "test.*\.py" | xargs rg "ControlledDict"
# Look for the actual implementation to understand the context better
ast-grep --pattern 'class ControlledDict {
$$$
update($$$) {
$$$
}
$$$
}'
Length of output: 1272
Script:
#!/bin/bash
# Let's look at the specific test cases for update method
rg -B 5 -A 10 "def test.*update" tests/test_collections.py
# Also check the actual test file content to understand the test coverage
cat tests/test_collections.py
# Look for any related issues or PRs in the commit history
git log --grep="ControlledDict.*update" --grep="pymatgen.*4223" -i
Length of output: 7848
DanielYang59
force-pushed
the
fix-namespace-dict-with-iterator
branch
from
0aeb657 to
d54bde4
Compare
collections.ControlledDicthandling of Iterators viaupdatemethod, to fix Fix CI failure: bumpnetcdf4, replaceNamespacewith standarddictforio.abinit.pseudosmaterialsproject/pymatgen#4223montytest is pretty cheapSummary by CodeRabbit
New Features
ControlledDictto accept zipped lists for updates.Bug Fixes
updatemethod ofControlledDict.setdefaultmethod.zopenfunction to enforce explicit mode and encoding.Tests
ControlledDict.Namespacetests to reflect changes in expected behavior.