pathconverter: Add file:// prefix to paths converted to absolute

[mtdcr]

This is useful if a post-processing program is unable to understand absolute non-URI locations, e.g. by interpreting them relative to a host and erroring out instead of defaulting to the local filesystem, if no host (base URI) is known.

[facelessuser]

This currently breaks existing tests so those would need to be cleaned up first. I'll have to evaluate this as recall there was a reason why I didn't do this. Assuming tests get fixed and I don't come up with a reason forcing file:// to be undesirable, then I think it looks okay. I'll run some tests when I get time to confirm.

[mtdcr]

Tests should be fixed now, but the workflow needs another trigger.

[facelessuser]

Yep, thanks! I'll try to evaluate the change this weekend.

[mtdcr]

I guess I overestimated my ability to convert windows paths to file URLs. Sorry, I'll fix it tomorrow. :-)

[facelessuser]

No worries, that's why we have tests 🙂.

[mtdcr]

Added another commit. Let me know what you think.

Btw., I wondered what path = util.path2url(path) is supposed to do in the code path changed by my last commit. IMO, urlunparse should be able to handle all quoting, and removing this line would probably allow to drop the special case for not util.is_win() in TestWindowsAbs, as all platforms would generate the same result.

[facelessuser]

I will have to look at this closely. I have projects relying on this support, and I'll have to assess potential breakage and such. I'll let you know when I have a chance to run this through various implementations. I'll run it through various browsers and through various in-app webviews. It is maybe some of these app-specific scenarios that I'm worried about breakage.

Particularly this project generates in-app docs and such that are displayed via some webview on various platforms: https://github.com/facelessuser/Rummage. Some of these webview implementations are finicky to say the least. This may not be a quick review, but I need to make sure it doesn't break anything I'm currently using it for as that was the whole reason behind me creating the extension in the first place.

[facelessuser]

Btw., I wondered what path = util.path2url(path) is supposed to do in the code path changed by my last commit. IMO, urlunparse should be able to handle all quoting, and removing this line would probably allow to drop the special case for not util.is_win() in TestWindowsAbs, as all platforms would generate the same result.

For reference, this is the change that added this: #408

Originally, it was added the default protocol notation. Here you are doing the other option and forcing file://. I'm still considering whether it is better to place file:// behind a configuration option. I'll know better when I get the chance to evaluate this more. I guess, if you did make this optional, it would relieve some of the additional need for stringent testing on my end as I have concerns about things breaking some projects downstream.

What I do know, is the original default protocol did not always work, so striping (as we currently do) or adding file:// was the way to go, what I am uncertain of is whether forcing file:// will break some expectations downstream.

[mtdcr]

Opt-in is fine. Added some commits with new tests and documentation.

[facelessuser]

Windows is failing as you really need pathname2url to be called on it which path2url was doing. I'm not sure why you are avoiding it. It seemed that it was undesirable because we had the test distinguish between nix and windows, but that is just how from urllib.request import pathname2url works. It has different behavior for nix and Windows because paths in these different systems have different behaviors. Windows doesn't allow : in file names, only drive letters, so it is accepted in drive letters, but fails if used anywhere else in the path. Unix/Linux escapes the colon. That's just how it works. Please continue using the path2url step.

[mtdcr]

Yes, I misunderstood how the function works on Windows.

[facelessuser]

No worries, I had to learn about these corner cases myself as well.

[facelessuser]

@gir-bot lgtm

[mtdcr]

Thank you, @facelessuser!