Use POSIX separators in file-protocol paths#1521
Conversation
dimitri-yatsenko
left a comment
There was a problem hiding this comment.
Nice catch, and the right fix. .as_posix() on the file branch of _full_path also completes a convention already used elsewhere in this file — get_url and normalize_to_url both force forward slashes for Windows via .replace(chr(92), '/'), and _full_path's file branch was the one spot still emitting native separators. Every file-protocol operation routes through _full_path (put_file/get_file/put/get_buffer/exists/isdir/remove/size/open/put_folder/get_url), so this single change covers all consumers, and fsspec's LocalFileSystem accepts forward-slash (incl. drive-letter) paths on Windows — no risk to the read/write paths. The PureWindowsPath monkeypatch makes the test platform-independent (runs on the Linux CI) and it fails pre-fix — nice.
LGTM to merge. One thing worth confirming, and one FYI:
Confirm the end-to-end GC path (not blocking, but worth a line). The new test asserts the _full_path separator contract, but it doesn't exercise the actual misclassification (list_hash_paths/list_schema_paths → _is_covered). That matters because the CI matrix has no Windows runner, and on Linux .as_posix() == str(...), so nothing automated exercises the failing path. Also, #1520 describes the mechanism as file_path.replace(full_root, ""), but the code on master uses a length slice — file_path[len(full_root):].lstrip("/") (gc.py:212, 261) — which is largely insensitive to separator style. So it'd be good to either add a small integration test (walk a file store with a Windows-style _full_path and assert the relative paths match a reference set) or confirm the manual Win11 collect() repro now passes end-to-end — just so we're sure separators are the true root cause rather than, say, a relative-vs-absolute prefix mismatch.
FYI: added the bug label and the v2.3.3 milestone so this data-loss fix lands in the release-drafter notes.
Nit (non-blocking): this file now has two idioms for the same thing — .as_posix() (new) vs str(...).replace(chr(92), '/') (lines 83, 462). .as_posix() is cleaner; worth unifying in a follow-up.
|
Opened #1522 to add a Windows unit-test leg to CI ( That addresses the CI half of the coverage note above. The end-to-end GC integration test (walking a |
|
I've closed #1520 (comment). datajoint-python/src/datajoint/gc.py Lines 293 to 294 in afc350f I've updated the PR title and description to reflect this. |
|
Re-reviewed after the reframe (the #1520 GC-misclassification rationale withdrawn, PR now justified by The reframed bug is real.
This also retroactively fits the #1520 withdrawal: No regression. Every file-protocol consumer of Test. Passes on the branch; I confirmed it fails pre-fix ( One open item (non-blocking): the test asserts only the Minor: two forward-slash idioms now coexist — |
This PR ensures
StorageBackend._full_path()always return POSIX-style (/) separators for the file protocol, instead of OS-native ones.It indirectly fixes
delete_schema_path's empty parent dir pruning, which silently no-ops on backslash paths:datajoint-python/src/datajoint/gc.py
Lines 293 to 294 in afc350f