Fix csv-view path and row display behavior#57
Conversation
|
Before I take this out of draft:
Fix those, and it’s good to go! |
📝 WalkthroughWalkthroughModified ChangesCSV path handling with fallback resolution
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…y-43 # Conflicts: # tests/test_data.py
|
Follow-up verification:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_data.py (1)
89-102: ⚡ Quick winConsider adding a test for Windows paths with spaces.
The current test validates Windows path handling and truncation, but the path used (
tmp_path / "users.csv") may not contain spaces. Real-world Windows paths commonly include spaces (e.g.,C:\Program Files\...,C:\Users\John Doe\...).Adding a test that creates a directory with spaces on Windows would validate the interaction of both fallback mechanisms (backslash handling + space handling).
Example test structure
`@pytest.mark.skipif`(sys.platform != "win32", reason="Windows path with spaces regression") def test_run_csv_view_handles_windows_paths_with_spaces(tmp_path: Path) -> None: from trushell.commands.data import run_csv_view directory = tmp_path / "Program Files" directory.mkdir() file_path = directory / "users.csv" file_path.write_text("ID,Name\n1,Ada\n", encoding="utf-8") output = _strip_ansi(run_csv_view(str(file_path))) assert "ID" in output assert "Name" in output assert "Ada" in output🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_data.py` around lines 89 - 102, Add a new Windows-only test to cover paths that include spaces: create a directory with a space in its name inside tmp_path, write a small CSV file there, call run_csv_view with str(file_path) (wrapped with _strip_ansi as in test_run_csv_view_handles_unquoted_windows_paths) and assert the expected headers and row values appear; name the test function test_run_csv_view_handles_windows_paths_with_spaces and use the same pytest.mark.skipif(sys.platform != "win32", ...) guard so it only runs on Windows.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/test_data.py`:
- Around line 89-102: Add a new Windows-only test to cover paths that include
spaces: create a directory with a space in its name inside tmp_path, write a
small CSV file there, call run_csv_view with str(file_path) (wrapped with
_strip_ansi as in test_run_csv_view_handles_unquoted_windows_paths) and assert
the expected headers and row values appear; name the test function
test_run_csv_view_handles_windows_paths_with_spaces and use the same
pytest.mark.skipif(sys.platform != "win32", ...) guard so it only runs on
Windows.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4bd1f94d-06f4-4568-80f1-78d62b1efcbe
📒 Files selected for processing (2)
tests/test_data.pytrushell/commands/data.py
Summary:
tests/test_data.pycoverage, including the short-row padding regression test, after resolving the merge conflict.Validation:
python -m pytest tests/test_data.py -qmain.Related issue:
Risk: