Skip to content

Fix: Use os.pathsep for TROVE_PATHS splitting to support Windows#8

Open
Ged-fi wants to merge 1 commit into
crunchtools:mainfrom
Ged-fi:fix-windows-paths
Open

Fix: Use os.pathsep for TROVE_PATHS splitting to support Windows#8
Ged-fi wants to merge 1 commit into
crunchtools:mainfrom
Ged-fi:fix-windows-paths

Conversation

@Ged-fi

@Ged-fi Ged-fi commented Mar 30, 2026

Copy link
Copy Markdown

Fixes issue where Windows drive letters (like C:) were improperly split when parsing TROVE_PATHS.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/mcp_trove_crunchtools/config.py Outdated
Comment on lines +94 to +98
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()]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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) == expected

@Ged-fi Ged-fi force-pushed the fix-windows-paths branch from 65427d8 to 92ced6d Compare March 30, 2026 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant