feat: ensure user home dirs are expanded in paths #7
Conversation
WalkthroughThis pull request introduces new test functions in the Changes
Possibly related PRs
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
typeline/_writer.py (1)
Line range hint
119-127: Document the path expansion behavior.Consider adding a note in the docstring about the automatic expansion of user home directories (e.g.,
~)."""Construct a delimited data writer from a file path. Args: path: the path to the file to write delimited data to. + Supports expansion of user home directory (e.g., "~/data/file.csv"). record_type: the type of the object we will be writing. """typeline/_reader.py (2)
219-219: LGTM! Good addition of home directory expansion.The addition of
expanduser()is a good practice that makes the API more user-friendly by supporting common path shortcuts like~.Consider also supporting environment variables in paths using
expandvars():- handle = Path(path).expanduser().open("r") + handle = Path(path).expanduser().expandvars().open("r")
Line range hint
208-220: Document the path expansion behavior.The docstring should mention that the path supports user home directory expansion.
Add this information to the
pathparameter description:Args: - path: the path to the file to read delimited data from. + path: the path to the file to read delimited data from. Supports user home directory expansion (e.g., '~'). record_type: the type of the object we will be writing. header: whether we expect the first line to be a header or not. comment_prefixes: skip lines that have any of these string prefixes.tests/test_reader.py (1)
289-304: LGTM! Consider adding more edge casesThe test effectively verifies the handling of old-style optional types with good coverage of regular values, null values, and nested structures.
Consider enhancing the test coverage with additional cases:
- Invalid data (e.g., string where int is expected)
- Empty string vs null handling
- Type hints for the function parameters (tmp_path: Path)
Here's a suggested enhancement:
-def test_reader_can_read_old_style_optional_types(tmp_path: Path) -> None: +def test_reader_can_read_old_style_optional_types(tmp_path: Path) -> None: """Test that the reader can read old style optional types.""" @dataclass class MyMetric: field1: float field2: Optional[int] field3: Optional[list[int]] - (tmp_path / "test.txt").write_text("0.1,1,null\n0.2,null,'[1,2,3]'\n") + (tmp_path / "test.txt").write_text( + "0.1,1,null\n" + "0.2,null,'[1,2,3]'\n" + "0.3,,null\n" # Empty string case + "0.4,invalid,null\n" # Invalid int case + ) with CsvRecordReader.from_path(tmp_path / "test.txt", MyMetric, header=False) as reader: - record1, record2 = list(iter(reader)) + records = list(iter(reader)) + assert len(records) == 2 # Should only get valid records - assert record1 == MyMetric(0.1, 1, None) - assert record2 == MyMetric(0.2, None, [1, 2, 3]) + assert records[0] == MyMetric(0.1, 1, None) + assert records[1] == MyMetric(0.2, None, [1, 2, 3]) + + # Verify invalid records raise appropriate exceptions + with pytest.raises(ValidationError): + list(CsvRecordReader.from_path( + tmp_path / "test.txt", + MyMetric, + header=False, + skip_errors=False + ))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
tests/test_reader.py(2 hunks)tests/test_writer.py(2 hunks)typeline/_reader.py(1 hunks)typeline/_writer.py(1 hunks)
🔇 Additional comments (4)
tests/test_writer.py (2)
4-4: LGTM!
The Optional import is correctly placed with other typing imports and is necessary for the new test function.
140-153: LGTM! Well-structured test implementation
The test is well-implemented with:
- Clear test cases covering both None and non-None scenarios for different Optional types
- Proper assertions verifying the CSV output format
- Good documentation
typeline/_writer.py (1)
128-128: LGTM! Verify consistent behavior with reader.
The addition of expanduser() correctly implements user home directory expansion, matching the behavior in DelimitedRecordReader.
Let's verify the consistent implementation across reader/writer:
✅ Verification successful
Path expansion implementation verified as consistent
The verification confirms that both DelimitedRecordReader and DelimitedRecordWriter consistently use expanduser() for handling user home directory paths:
typeline/_reader.py:Path(path).expanduser().open("r")typeline/_writer.py:Path(path).expanduser().open("w")
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent path handling between reader and writer
# Expected: Both should use expanduser() in from_path method
# Check for expanduser usage in reader and writer
rg "expanduser\(\)" --type py
Length of output: 191
tests/test_reader.py (1)
4-4: LGTM!
The Optional import is correctly placed with other typing imports and is necessary for the new test function.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
typeline/_data_types.py (1)
37-39: Enhance the docstring with more detailsThe implementation looks good and properly handles both old and new-style unions. Consider enhancing the docstring with:
- Parameter description
- Return value description
- Examples of usage
def is_union(_type: type | str | Any) -> bool: - """Return if this type is a union of types or not.""" + """Check if the given type is a union of types. + + Args: + _type: The type to check. Can be a type object, string representation + of a type, or Any. + + Returns: + bool: True if the type is a union (either UnionType or _UnionGenericAlias), + False otherwise. + + Examples: + >>> is_union(int | str) + True + >>> is_union(int) + False + """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
tests/test_reader.py(2 hunks)typeline/__init__.py(2 hunks)typeline/_data_types.py(2 hunks)typeline/_reader.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_reader.py
- typeline/_reader.py
🔇 Additional comments (3)
typeline/__init__.py (2)
3-3: LGTM! Import statement follows consistent style
The new import follows the established pattern and is properly grouped with related type-handling imports.
20-20: Verify the usage of newly exposed is_union function
The addition to __all__ properly exposes the function. Let's verify its usage pattern across the codebase to ensure it's being used as intended.
typeline/_data_types.py (1)
8-10: Verify the necessity of using internal typing API
The use of _UnionGenericAlias (an internal typing API) might make the code brittle across Python versions. Consider if there's a more stable public API alternative.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
test_reader_can_read_old_style_optional_typesto validate reading optional types.test_writer_can_write_old_style_optional_typesto validate writing optional types.is_unionfunction.