Skip to content

Fix file+noindex URI usage on Windows#10746

Merged
mergify[bot] merged 1 commit intohaskell:masterfrom
jasagredo:js/local-noindex-2
Jan 16, 2025
Merged

Fix file+noindex URI usage on Windows#10746
mergify[bot] merged 1 commit intohaskell:masterfrom
jasagredo:js/local-noindex-2

Conversation

@jasagredo
Copy link
Collaborator

@jasagredo jasagredo commented Jan 13, 2025

Concluded on using file+noindex:C:/some/path on Windows as discussed on Matrix.

Template Α: This PR modifies behaviour or interface

Include the following checklist in your PR:

@jasagredo jasagredo force-pushed the js/local-noindex-2 branch 3 times, most recently from 89c9df2 to 19fad89 Compare January 13, 2025 17:04
@jasagredo jasagredo marked this pull request as ready for review January 13, 2025 17:10
Copy link
Collaborator

@ulysses4ever ulysses4ever left a comment

Choose a reason for hiding this comment

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

Terrific! Any chance for a test?

@jasagredo
Copy link
Collaborator Author

I can't push because Git operations on github are down at the moment. Will push tomorrow.

As for the tests @ulysses4ever, the current test-suite makes use of this both by the roundtrip of project configurations and by some tests that make use of local+noindex repos such as the ones added recently by @9999years. I consider that sufficient testing.

Copy link
Collaborator

@9999years 9999years left a comment

Choose a reason for hiding this comment

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

Windows paths are so tricky

@ulysses4ever
Copy link
Collaborator

the current test-suite makes use of this both by the roundtrip of project configurations and by some tests that make use of local+noindex repos such as the ones added recently by @9999years. I consider that sufficient testing.

It's just that if anything in the current setup exercises this functionality, I expect the setup to need an update after these changes.

@jasagredo
Copy link
Collaborator Author

The previous PR that was merged did change the OutputNormalizer. It then worked with //./ paths. Now it works with file+noindex:C:/ paths.

@jasagredo jasagredo force-pushed the js/local-noindex-2 branch 2 times, most recently from 5ee181b to 70afe9b Compare January 14, 2025 00:37
@mergify mergify bot added the ready and waiting Mergify is waiting out the cooldown period label Jan 14, 2025
@jasagredo
Copy link
Collaborator Author

jasagredo commented Jan 14, 2025

One could argue that the right syntax would be:

file+noindex:///C:/foo

But that is not understood by network-uri. It is however what .NET does:

➜ [System.Uri]'C:/foo.txt'

AbsolutePath   : C:/foo.txt
AbsoluteUri    : file:///C:/foo.txt
...

In any case .NET also supports the syntax we use here so it should be fine:

➜ [System.Uri]'file:C:/foo.txt'

AbsolutePath   : C:/foo.txt
AbsoluteUri    : file:///C:/foo.txt
...

Probably the right thing to do would be to make use of file-uri for file+noindex paths, but that would require us to not first read it as a network-uri. The good news is that file-uri understands both versions and both are "the same" so once we make this change and recommend file:/// the previous code won't break:

ghci> parseFileURI ExtendedWindows  "file:///C:/foo.txt"
Right (FileURI {fileAuth = Nothing, filePath = "C:/foo.txt"})
ghci> parseFileURI ExtendedWindows  "file:C:/foo.txt"
Right (FileURI {fileAuth = Nothing, filePath = "C:/foo.txt"})

cc @hasufell

@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Jan 16, 2025
@Mikolaj Mikolaj force-pushed the js/local-noindex-2 branch from 70afe9b to 9a0e8a4 Compare January 16, 2025 00:50
@mergify mergify bot merged commit 4edd5c4 into haskell:master Jan 16, 2025
46 checks passed
@Mikolaj
Copy link
Member

Mikolaj commented Mar 28, 2025

@mergify backport 3.14

@mergify
Copy link
Contributor

mergify bot commented Mar 28, 2025

backport 3.14

✅ Backports have been created

Details

@ulysses4ever
Copy link
Collaborator

The title of this PR doesn't match the commit title which makes it very confusing because there was another PR with a title like this one (#10728).

Sorry for a silly question, but is "local+noindex" the same as "file+noindex"?

2025-03-28_09-03-1743167455

@jasagredo
Copy link
Collaborator Author

It should always have been file+noindex. There is no local+noindex except on a single comment. Therefore the title of the commit is wrong.

mergify bot added a commit that referenced this pull request Mar 28, 2025
* Fix local+noindex repos on Windows

(cherry picked from commit 9a0e8a4)

# Conflicts:
#	Makefile

* fixup! Makefile doctest rule was changed back to what it once was

* fixup! Fix local+noindex repos on Windows

---------

Co-authored-by: Javier Sagredo <jasataco@gmail.com>
Co-authored-by: Artem Pelenitsyn <a.pelenitsyn@gmail.com>
Co-authored-by: brandon s allbery kf8nh <allbery.b@gmail.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days merge me Tell Mergify Bot to merge ready and waiting Mergify is waiting out the cooldown period

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants