Richer AccessorPath API #63
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request enhances the AccessorPath API by introducing floor division operators for path navigation, removing deprecated methods, and updating the get method to return values instead of paths. The changes aim to provide a more intuitive and Pythonic interface for working with paths.
Changes:
- Added
__floordiv__operator (//) for accessing child paths that exist, raising KeyError if they don't - Removed deprecated methods (
getkey,iter,iteritems,content,open) that had been warning users to use alternative approaches - Changed
getmethod from returning paths to returning values, aligning with dict-like behavior and updating docstrings accordingly
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| pathable/paths.py | Implements floor division operators, adds exists() method, removes deprecated methods, changes get() to return values, improves docstrings |
| tests/unit/test_paths.py | Adds TestLookupPathFloorDiv class with tests for the floor division operator, updates existing test to use // operator |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pathable/accessors.py
Outdated
| return { | ||
| 'type': type(node), | ||
| 'length': len(node), |
There was a problem hiding this comment.
This implementation will raise a TypeError when stat() is called on a path pointing to a leaf value (e.g., string, number, boolean, None) that doesn't support len(). The previous implementation correctly checked if the node was a Mapping or list before calling len(). Consider adding type checking or wrapping len() in a try-except block to handle leaf values that don't support length operations.
| return { | |
| 'type': type(node), | |
| 'length': len(node), | |
| try: | |
| length = len(node) # type: ignore[arg-type] | |
| except TypeError: | |
| length = None | |
| return { | |
| 'type': type(node), | |
| 'length': length, |
| return { | ||
| 'type': type(node), | ||
| 'length': len(node), | ||
| } |
There was a problem hiding this comment.
The return value for the 'type' key has changed from a string literal ('mapping' or 'list') to a type object (type(node)). This is a breaking API change that could affect consumers expecting string values. If this change is intentional, it should be clearly documented. Additionally, this will now include the type for all node values, not just Mappings and lists, which changes the semantics of when stat() returns a result versus None.
| return { | |
| 'type': type(node), | |
| 'length': len(node), | |
| } | |
| if isinstance(node, Mapping): | |
| return { | |
| 'type': 'mapping', | |
| 'length': len(node), | |
| } | |
| if isinstance(node, list): | |
| return { | |
| 'type': 'list', | |
| 'length': len(node), | |
| } | |
| return None |
| def get(self, key: K, default: Any = None) -> Any: | ||
| """Return the child path for key if key is in the path, else default.""" | ||
| warnings.warn( | ||
| "'get' method is deprecated. Use 'key in path' and 'path / key' instead.", | ||
| DeprecationWarning, | ||
| stacklevel=2, | ||
| ) | ||
| if key in self: | ||
| return self / key | ||
| return default | ||
|
|
||
| @contextmanager | ||
| def open(self) -> Any: | ||
| """Open the path.""" | ||
| warnings.warn( | ||
| "'open' method is deprecated. Use 'read_value' instead.", | ||
| DeprecationWarning, | ||
| stacklevel=2, | ||
| ) | ||
| yield self.read_value() | ||
| """Return the value for key if key is in the path, else default.""" | ||
| try: | ||
| return self[key] | ||
| except KeyError: | ||
| return default |
There was a problem hiding this comment.
The semantics of the get() method have completely changed. The old deprecated implementation returned a child Path object (self / key), while the new implementation returns the value at that path (self[key], which calls read_value()). This is a breaking change that fundamentally alters the method's behavior. Consider either: (1) keeping the old path-returning behavior for backwards compatibility, or (2) renaming this method to something like get_value() to make the semantic difference clear, or (3) clearly documenting this breaking change in migration documentation.
| def exists(self) -> bool: | ||
| """Check if the path exists.""" | ||
| return self.accessor.stat(self.parts) is not None |
There was a problem hiding this comment.
The new exists() method lacks test coverage. Given that this codebase has comprehensive test coverage for other methods (as evidenced by the extensive test classes in this file), the exists() method should have corresponding tests to verify: (1) it returns True for existing paths, (2) it returns False for non-existing paths, and (3) edge cases like empty paths or invalid path structures.
1 participant
This pull request introduces enhancements and cleanup to the path handling logic in
pathable/paths.py, focusing on improving the interface for accessing and manipulating child paths, as well as updating and expanding the related test coverage. The most significant changes include adding floor division operators for path navigation, removing deprecated methods, and updating tests to reflect the new interface.Path interface improvements
__floordiv__and__rfloordiv__methods to allow using the floor division operator (//) for accessing child paths, providing a more intuitive and Pythonic way to traverse paths. ([pathable/paths.pyR162-R213](https://github.com/p1c2u/pathable/pull/63/files#diff-97204c858188d235e2027743dbf615b39e3a1fb67cc1bf6168d2704fa068c165R162-R213))__getitem__andgetmethods to use the new floor division operator internally, ensuring consistent behavior and error handling. ([pathable/paths.pyR162-R213](https://github.com/p1c2u/pathable/pull/63/files#diff-97204c858188d235e2027743dbf615b39e3a1fb67cc1bf6168d2704fa068c165R162-R213))existsmethod for checking if a path exists, and improved docstrings for better clarity throughout the class. ([pathable/paths.pyR162-R213](https://github.com/p1c2u/pathable/pull/63/files#diff-97204c858188d235e2027743dbf615b39e3a1fb67cc1bf6168d2704fa068c165R162-R213))Deprecated method cleanup
getkey,iter,iteritems,content,open) to streamline the API and encourage use of the updated interface. ([pathable/paths.pyR162-R213](https://github.com/p1c2u/pathable/pull/63/files#diff-97204c858188d235e2027743dbf615b39e3a1fb67cc1bf6168d2704fa068c165R162-R213))Test suite updates
TestLookupPathFloorDiv, ensuring correct error handling and value retrieval. ([tests/unit/test_paths.pyR809-R841](https://github.com/p1c2u/pathable/pull/63/files#diff-c18e2e75a029ba4cabcc608ca2fe3ef7b95551458089c10c554e10b5c239baacR809-R841))//operator instead of the oldgetmethod for accessing child paths. ([tests/unit/test_paths.pyL829-R862](https://github.com/p1c2u/pathable/pull/63/files#diff-c18e2e75a029ba4cabcc608ca2fe3ef7b95551458089c10c554e10b5c239baacL829-R862))