Skip to content

Fix Windows Absolute Path Parsing and Remove HTTP Assumption#1837

Merged
mre merged 8 commits into
masterfrom
absolute-local-windows-paths
Mar 15, 2026
Merged

Fix Windows Absolute Path Parsing and Remove HTTP Assumption#1837
mre merged 8 commits into
masterfrom
absolute-local-windows-paths

Conversation

@mre

@mre mre commented Sep 3, 2025

Copy link
Copy Markdown
Member

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 foo would previously have been automatically converted to http://foo/, which is a bold and mostly incorrect presumption. You can read more about it in the issue here.

Key changes in this PR:

  • 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 and #1595

@mre mre force-pushed the absolute-local-windows-paths branch 5 times, most recently from ac066ce to c1d2eee Compare September 4, 2025 10:39
This was referenced Sep 6, 2025
Closed
Closed
@katrinafyi

katrinafyi commented Sep 8, 2025

Copy link
Copy Markdown
Member

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.

@mre

mre commented Sep 8, 2025

Copy link
Copy Markdown
Member Author

Thanks for the feedback; I'll look into it.

@mre mre added the triage label Sep 23, 2025
@mre

mre commented Oct 9, 2025

Copy link
Copy Markdown
Member Author

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:

  • rebase
  • support lowercase Windows paths
  • Try to unify Unix and Windows logic
  • Try to use PathBuf for Windows instead of WindowsPath

@thomas-zahner thomas-zahner force-pushed the master branch 2 times, most recently from fcdf77c to e0912ab Compare October 21, 2025 12:53
@mre mre force-pushed the absolute-local-windows-paths branch from c1d2eee to 07b569a Compare December 16, 2025 11:14
katrinafyi
katrinafyi reviewed Dec 17, 2025
View reviewed changes
Comment thread lychee-lib/src/types/input/input.rs
Comment thread lychee-lib/src/types/error.rs
Comment thread lychee-lib/src/types/input/source.rs Outdated
Comment thread .github/workflows/ci.yml Outdated
@katrinafyi katrinafyi mentioned this pull request Jan 28, 2026
Merged
@mre mre force-pushed the absolute-local-windows-paths branch 2 times, most recently from 98bc4b6 to 527fde8 Compare February 5, 2026 15:48
katrinafyi
katrinafyi reviewed Feb 22, 2026
View reviewed changes

@katrinafyi katrinafyi left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

To get this merged, it might be easier to enable windows CI and tests in a follow-up PR. Removing http assumption by itself is an important fix that should not be stuck behind windows. I think we need a proper strategy for writing cross platform tests.

@mre mre force-pushed the absolute-local-windows-paths branch from b18fe64 to b5b5c03 Compare March 13, 2026 21:18
@mre

mre commented Mar 13, 2026

Copy link
Copy Markdown
Member Author

I took another stab at it today and addressed the outstanding review feedback. Moved out the Windows CI stuff to a different branch. @katrinafyi, can you take another look?

mre
mre commented Mar 13, 2026
View reviewed changes
Comment thread lychee-bin/tests/cli.rs
katrinafyi
katrinafyi approved these changes Mar 14, 2026
View reviewed changes

@katrinafyi katrinafyi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good, a few small comments.

Comment thread lychee-lib/src/types/error.rs Outdated
Comment thread lychee-lib/src/types/input/source.rs Outdated
Comment thread lychee-lib/src/types/input/source.rs Outdated
@mre

mre commented Mar 14, 2026

Copy link
Copy Markdown
Member Author

Nice! I've made those changes. It's much cleaner now. Thanks for the quick turnaround. 👍

katrinafyi
katrinafyi approved these changes Mar 14, 2026
View reviewed changes

@katrinafyi katrinafyi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

looks good on my end!

@mre mre merged commit fe2cd9f into master Mar 15, 2026
8 checks passed
@mre mre deleted the absolute-local-windows-paths branch March 15, 2026 09:55
@mre mre mentioned this pull request Mar 14, 2026
Merged
@mre

mre commented Mar 15, 2026

Copy link
Copy Markdown
Member Author

Thanks for reviewing @katrinafyi. Went ahead and merged this. @thomas-zahner fyi.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thomas-zahner thomas-zahner thomas-zahner left review comments

+1 more reviewer

@katrinafyi katrinafyi katrinafyi approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Absolute local paths on Windows not recognized

3 participants

@mre @katrinafyi @thomas-zahner