Fix: Use os.pathsep for TROVE_PATHS splitting to support Windows#8
Fix: Use os.pathsep for TROVE_PATHS splitting to support Windows#8Ged-fi wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the _parse_paths function in config.py to use os.pathsep instead of a hardcoded colon, improving cross-platform compatibility for directory path parsing. The reviewer suggested adding unit tests to verify this OS-dependent logic and provided a comprehensive code example for testing both POSIX and Windows scenarios using mocks.
| def _parse_paths(raw: str) -> list[str]: | ||
| """Parse OS-dependent separated directory paths.""" | ||
| if not raw.strip(): | ||
| return [] | ||
| return [p.strip() for p in raw.split(os.pathsep) if p.strip()] |
There was a problem hiding this comment.
While this change correctly uses os.pathsep for cross-platform compatibility, the _parse_paths function lacks unit tests. Given that its behavior is OS-dependent, adding tests would be valuable to ensure it works as expected on different platforms (e.g., POSIX and Windows) and to prevent future regressions.
Consider adding a new test file, for example tests/test_config.py, with tests that mock os.pathsep to verify the parsing logic.
Here's an example of what these tests could look like:
# In a new file tests/test_config.py
import os
from unittest import mock
from mcp_trove_crunchtools.config import _parse_paths
def test_parse_paths_posix():
"""Test path parsing with POSIX-style separator."""
with mock.patch('os.pathsep', ':'):
raw_paths = "/home/user/docs:/data/files"
expected = ["/home/user/docs", "/data/files"]
assert _parse_paths(raw_paths) == expected
def test_parse_paths_windows():
"""Test path parsing with Windows-style separator."""
with mock.patch('os.pathsep', ';'):
raw_paths = r"C:\\Users\\user\\docs;D:\\data"
expected = [r"C:\\Users\\user\\docs", r"D:\\data"]
assert _parse_paths(raw_paths) == expected
def test_parse_paths_empty_and_whitespace():
"""Test that empty or whitespace-only input returns an empty list."""
assert _parse_paths("") == []
assert _parse_paths(" ") == []
def test_parse_paths_with_empty_segments():
"""Test that empty segments in the path string are ignored."""
with mock.patch('os.pathsep', ':'):
raw_paths = "/path/one::/path/two:"
expected = ["/path/one", "/path/two"]
assert _parse_paths(raw_paths) == expected65427d8 to
92ced6d
Compare
Fixes issue where Windows drive letters (like C:) were improperly split when parsing TROVE_PATHS.