Refactor code#11
Conversation
📝 WalkthroughWalkthroughBumps version and Ruff line length; adds docstrings; introduces iterator integration constants; changes flatten/iterate/drop_while semantics; tightens numeric validation and empty-stream handling; hardens FileStream initialization cleanup; improves DictItem hashing errors; removes a test fixture; and adds/expands tests for edge and error cases. Changes
Sequence Diagram(s)Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🤖 Fix all issues with AI agents
In @pyrio/utils/dict_item.py:
- Around line 31-36: The current __hash__ implementation violates the
hash/equality contract for DictItem when values are unhashable; update
DictItem.__hash__ to attempt hashing (e.g., hash((self._key, self._value))) and
if that raises TypeError re-raise a TypeError so instances with unhashable
values become unhashable (do not fall back to id(self._value)); reference the
DictItem class and its __hash__ and __eq__ methods and ensure the error message
clearly states that DictItem is unhashable when its value is unhashable.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
pyproject.tomlpyrio/decorators/handler.pypyrio/decorators/mapper.pypyrio/iterators/itertools_mixin.pypyrio/iterators/stream_generator.pypyrio/streams/base_stream.pypyrio/streams/file_stream.pypyrio/utils/dict_item.pytests/conftest.pytests/test_dict_item.pytests/test_file_stream.pytests/test_itertools_mixin.pytests/test_stream.py
💤 Files with no reviewable changes (1)
- tests/conftest.py
🧰 Additional context used
🧬 Code graph analysis (8)
pyrio/decorators/handler.py (1)
pyrio/streams/base_stream.py (1)
BaseStream(10-389)
tests/test_file_stream.py (4)
pyrio/streams/file_stream.py (2)
FileStream(62-260)process(92-94)pyrio/iterators/stream_generator.py (3)
map(24-27)filter(17-21)flat_map(37-40)pyrio/streams/base_stream.py (8)
map(48-51)to_tuple(274-276)filter(43-46)flat_map(58-61)BaseStream(10-389)iterable(24-27)iterable(30-31)close(373-377)pyrio/streams/stream.py (1)
Stream(7-54)
tests/test_dict_item.py (2)
pyrio/utils/dict_item.py (3)
value(13-14)DictItem(1-36)key(9-10)tests/conftest.py (2)
json_dict(38-46)nested_json(15-34)
tests/test_itertools_mixin.py (3)
pyrio/streams/stream.py (1)
Stream(7-54)pyrio/iterators/itertools_mixin.py (1)
use(13-24)pyrio/streams/base_stream.py (1)
to_list(270-272)
pyrio/streams/file_stream.py (1)
pyrio/streams/base_stream.py (3)
iterable(24-27)iterable(30-31)close(373-377)
pyrio/streams/base_stream.py (3)
pyrio/utils/dict_item.py (1)
DictItem(1-36)pyrio/streams/stream.py (1)
of_nullable(16-20)pyrio/exceptions/exception.py (1)
UnsupportedTypeError(13-14)
tests/test_stream.py (3)
pyrio/streams/stream.py (3)
Stream(7-54)of(11-13)empty(23-25)pyrio/streams/base_stream.py (15)
filter_map(53-56)to_list(270-272)reduce(217-231)head(112-117)to_tuple(274-276)sum(88-91)average(93-96)take_last(140-145)sort(147-153)map(48-51)reverse(155-161)to_dict(282-301)none_match(192-194)to_string(315-317)collect(240-268)pyrio/utils/dict_item.py (2)
value(13-14)key(9-10)
pyrio/iterators/stream_generator.py (4)
pyrio/decorators/mapper.py (1)
map_dict_items(7-23)pyrio/streams/base_stream.py (5)
concat(33-36)iterable(24-27)iterable(30-31)map(48-51)enumerate(209-215)pyrio/streams/stream.py (1)
iterate(28-30)pyrio/utils/dict_item.py (1)
key(9-10)
🔇 Additional comments (47)
pyproject.toml (1)
3-3: LGTM!Version bump to 1.6.0 and line-length adjustment to 100 are appropriate for this refactoring release. The 100-character limit aligns well with common Python conventions.
Also applies to: 45-45
tests/test_itertools_mixin.py (2)
284-287: Good catch fixing the test name.Renaming
rest_view_negative_steptotest_view_negative_stepensures pytest properly discovers and runs this test. The previous name would have been silently skipped during test runs.
10-12: LGTM!Formatting adjustments align with the new 100-character line limit.
pyrio/decorators/handler.py (1)
35-56: LGTM!The docstrings accurately describe the decorator behaviors:
pre_call: class-level decorator that wraps all callable methodshandle_consumed: prevents re-use of consumed streams and ensures cleanup after terminal operationsThe
# pragma: no coveron line 56 is appropriate for the defensive fallback branch.pyrio/iterators/itertools_mixin.py (2)
7-11: LGTM!Extracting the function lists to class-level constants (
NO_SIGNATURE_FUNCTIONS,NO_KWARGS_FUNCTIONS) improves maintainability and allows subclasses to extend or override these lists if needed.
27-27: LGTM!Using
self.NO_SIGNATURE_FUNCTIONSandself.NO_KWARGS_FUNCTIONSinstead of inline literals centralizes the configuration and maintains consistency with the new class-level constants.Also applies to: 48-48
pyrio/iterators/stream_generator.py (4)
6-8: LGTM!Clear class-level docstring explaining the lazy evaluation pattern.
42-49: LGTM!The
flattenimplementation correctly handles recursive flattening of nested iterables while preserving strings as atomic elements. The docstring accurately describes the behavior.
58-65: LGTM!The
iterategenerator correctly yields the seed first, then repeatedly applies the operation to produce subsequent values. The optional condition parameter allows for bounded iteration.
122-132: LGTM!The
drop_whileimplementation correctly handles the two-phase iteration: first skipping elements while the predicate is true, then yielding all remaining elements (including the first non-matching element that triggered the break).pyrio/streams/file_stream.py (2)
74-89: Excellent resource safety improvement.The try/except pattern ensures the file handle is properly closed if any exception occurs during file reading or stream initialization. Key points:
file_handler = Nonebefore the try block preventsUnboundLocalErrorin the except block- The check
file_handler is not None and not file_handler.closedis defensive and correct- Re-raising the exception preserves the original error context
147-155: LGTM!Method signature reformatting aligns with the 100-character line limit without changing functionality.
Also applies to: 192-194
pyrio/decorators/mapper.py (1)
7-9: LGTM!The docstring concisely describes the decorator's purpose. The implementation correctly transforms only
Mappingarguments while preserving other argument types.tests/test_dict_item.py (3)
42-46: LGTM!Formatting adjustment aligns with the updated line length configuration. No logic changes.
69-71: LGTM!Reformatted assertion for better readability.
81-89: LGTM!Good test coverage for the new
__hash__fallback behavior. The tests verify that:
hash()returns anintfor both hashable and unhashable values (lists, dicts)- Hash is consistent across multiple calls on the same object
Note that the consistency test (
hash(item) == hash(item)) will always pass for the same object within a single process run, which is the expected behavior being verified here.tests/test_file_stream.py (10)
70-75: LGTM!Formatting adjustment for lambda expression line wrapping.
103-108: LGTM!Improved readability with multiline chaining for the nested stream operations.
223-225: LGTM!Minor formatting adjustment.
336-338: LGTM!Formatting change for lambda expression.
405-407: LGTM!Assertion reformatted for readability.
437-439: LGTM!Formatting change for method chaining.
503-508: LGTM!Formatting adjustment for the save call with multiple options.
511-542: Good test for resource cleanup on initialization failure.The test properly verifies that the file handler is closed when an exception occurs during
FileStreaminitialization. The monkeypatch approach is appropriate for simulating the failure scenario.One minor observation: the test tracks close calls via
close_called.append(True)but assertslen(close_called) > 0. This is sufficient for the test's purpose.
545-556: Good test for stale temporary file cleanup.The test verifies that pre-existing
.tmpfiles are properly cleaned up during the save operation, which is important for robustness in case of previous interrupted writes.
559-579: Good test for atomic write failure recovery.The test properly verifies that:
- The temporary file is cleaned up when serialization fails
- The source file remains unchanged after the failure
This ensures the atomic write pattern is working correctly - either the write succeeds completely or the original file is preserved.
tests/test_stream.py (14)
106-111: LGTM!Formatting adjustment for line wrapping.
126-145: Excellent test coverage for reduce on iterators.Good addition of tests covering:
- Basic reduce on iterator
- Reduce with identity on iterator
- Empty iterator (with and without identity)
- Single element iterator
These tests verify that
reduceworks correctly with non-rewindable iterators.
219-222: Good test for negative head count validation.Matches the implementation in
base_stream.pythat raisesValueErrorfor negative counts.
391-410: Good test coverage for sum edge cases.Tests cover:
- Sum on iterators
- Sum with floats
- Sum with None values (filtered out)
- Sum with mixed types (raises)
- Sum when all values are None
425-447: Good test coverage for average edge cases.Tests mirror the sum tests and verify:
- Error message updated to reference "average" instead of "sum"
- Average on iterators
- Average with floats
- Average with None values
- Average with mixed types
- Average when all None
497-510: Good test coverage for take_last on iterators.Tests verify
take_lastworks correctly with:
- Regular iterators
- Empty iterators
- Empty iterators with default value
- Single element iterators
537-545: LGTM!Formatting adjustment for multi-key sort test.
569-577: LGTM!Formatting adjustment for reverse with comparator test.
582-584: LGTM!Formatting adjustment for complex pipeline test.
720-733: Good test coverage for none_match edge cases.Tests verify:
none_matchon empty stream returnsTrue(correct behavior - vacuously true)- Partial match, all match, and single match scenarios all return
False
833-838: LGTM!Formatting adjustment for to_dict_merger test.
941-949: Good test coverage for group_by edge cases.Tests verify:
- Empty stream returns empty dict
- Unconsumed groups can still yield keys correctly
959-961: LGTM!Formatting adjustment for to_string test.
1031-1034: LGTM!Formatting adjustment for parametrize decorator.
pyrio/streams/base_stream.py (7)
23-27: Good optimization for Mapping handling.Caching the tuple of
DictIteminstances avoids recreating them on each property access. This is more efficient and ensures consistent object identity across multiple accesses.
82-96: Good refactor: Extracted numeric validation into helper.The
_validate_numeric_datahelper cleanly handles:
- Type validation (int/float/None)
- Filtering out None values
- Dynamic error message with operation name
Both
sumandaveragenow share this validation logic, reducing duplication.
140-145: Good optimization for take_last.Using
deque(maxlen=1)is an efficient O(n) single-pass approach that works with any iterable, including generators that can't be indexed. This is preferable to converting to a list just to get the last element.
192-194: LGTM!The simplified
none_matchimplementation usingnot any(...)is semantically clearer and equivalent to the previous logic.
217-231: Good improvement to reduce for empty iterators.The try/except pattern for handling empty iterators is cleaner than checking length (which wouldn't work for generators). The behavior correctly:
- Returns
Optional.of_nullable(identity)when the stream is empty with an identity provided- Returns
Optional.of_nullable(None)when empty without identity (viaStopIteration)
233-237: LGTM!Formatting adjustment for readability.
303-313: LGTM!Error message reformatted to multiline for consistency with line length configuration.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pyrio/utils/dict_item.py (1)
31-37: Consider exception chaining for better debugging.The new error handling provides a clearer message, which is good. However, the original exception context is lost. Using
fromto chain exceptions preserves the traceback, which can help with debugging.♻️ Suggested improvement
def __hash__(self): try: return hash((self._key, self._value)) - except TypeError: + except TypeError as e: raise TypeError( f"unhashable type: 'DictItem' (value of type '{type(self._value).__name__}' is unhashable)" - ) + ) from etests/test_dict_item.py (1)
86-89: Consider strengthening the hash consistency test.The current test
hash(item) == hash(item)is trivially true since it compares the same call. A more meaningful test would verify the hash contract: equal objects must have equal hashes.♻️ Suggested improvement
@pytest.mark.parametrize("value", ["John", 42, (1, 2, 3)]) def test_dict_item_hash_consistency(value): item = DictItem(key="k", value=value) - assert hash(item) == hash(item) + item2 = DictItem(key="k", value=value) + # Equal items must have equal hashes (hash contract) + assert item == item2 + assert hash(item) == hash(item2)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pyrio/utils/dict_item.pytests/test_dict_item.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_dict_item.py (2)
pyrio/utils/dict_item.py (3)
value(13-14)DictItem(1-37)key(9-10)tests/conftest.py (2)
json_dict(38-46)nested_json(15-34)
🔇 Additional comments (3)
tests/test_dict_item.py (3)
42-45: LGTM!Formatting change for better readability. No functional impact.
69-71: LGTM!Formatting change for better readability.
92-102: LGTM!Good test coverage for the new error handling in
__hash__. The parameterized test covers lists, dicts, and nested structures, and correctly validates the error message format.
1 participant
Summary by CodeRabbit
Chores
Bug Fixes
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.