Fix Windows Absolute Path Parsing and Remove HTTP Assumption #1837
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mre
force-pushed
the
absolute-local-windows-paths
branch
from
f2dd6d8 to
fc853d6
Compare
This commit fixes issue #972 where Windows absolute paths like C:\path were incorrectly parsed as URLs with scheme C:. Key changes: - Added WindowsPath newtype with proper detection using pattern matching - Moved Windows path logic to separate submodule for better organization - Removed automatic HTTP assumption (foo -> http://foo/) - Added InvalidInput error type with helpful error messages - Updated all tests to reflect new behavior Fixes #972
mre
force-pushed
the
absolute-local-windows-paths
branch
from
fc853d6 to
3afb65e
Compare
- Replace From<WindowsPath> for PathBuf with as_path() method for better API - Move WindowsPath unit tests to the windows_path module for better organization - Fix unused import warning
mre
force-pushed
the
absolute-local-windows-paths
branch
from
a438f5f to
fc666f7
Compare
- Update test_inputs_without_scheme to expect failure for domain-like inputs - Refine Unix path detection logic to properly handle skip_missing behavior - File-like inputs (containing hyphens, slashes) are treated as paths for skip_missing - Domain-like inputs and simple words still fail with helpful error messages - Change WindowsPath::try_from to return Result with descriptive error messages All tests now pass including CLI integration tests.
mre
force-pushed
the
absolute-local-windows-paths
branch
from
5e502bf to
2e31400
Compare
Update test expectation to match new error message format. The error now occurs during file content reading rather than input parsing, which is the correct behavior for relative paths.
mre
force-pushed
the
absolute-local-windows-paths
branch
from
ac066ce to
c1d2eee
Compare
This was referenced Sep 6, 2025
|
I have quick comments. The first, and easiest, is that lowercase drive letters should be permitted. The second is that I think that the Unix and Windows logic should be unified so Windows also gains helpful hints like the full URL suggestion. The third is that (imo) it would be preferable to avoid custom WindowsPath parsing and logic. Windows paths are notoriously complicated. Is there a reason why using PathBuf on Windows is not sufficient? Is there a need to recognise windows paths on Unix platforms? It would also be nice if there was a CI job to run windows tests. |
|
Thanks for the feedback; I'll look into it. |
|
Sorry for dropping the ball on this. Just a reminder what needs to be done so that I can quickly jump back in when I find the time:
|
thomas-zahner
force-pushed
the
master
branch
2 times, most recently
from
fcdf77c to
e0912ab
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit fixes issue #972 where Windows absolute paths like C:\path were incorrectly parsed as URLs with scheme
C:.I also took that as an opportunity to finaly remove the assumption that non-existent inputs get parsed as HTTP URLs. So an input
foowould previously have been automatically converted tohttp://foo/, which is a bold and mostly incorrect presumption. You can read more about it in the issue here.Key changes in this PR:
Fixes #972 and #1595