Improve diagnostics for file URLs with hostnames #3630
Conversation
karwa
requested review from
abertelrud,
friedbunny,
neonichu and
tomerd
as code owners
|
Thanks for the PR, @karwa. I had indeed not considered the other failure cases, but I think we should also still represent the relative URL case with its own diagnostic, at least in its most simple form of the hostname component being |
|
@karwa do you need more information to finalize this? |
karwa
force-pushed
the
file-url-hostnames
branch
from
a537ae3 to
728ca96
Compare
|
@abertelrud @neonichu Sorry, I just totally forgot about it! 😓 I've added the relative path diagnostic back if the hostname is ".." |
| guard hostnameComponent.isEmpty else { | ||
| if hostnameComponent == ".." { | ||
| throw ManifestParseError.invalidManifestFormat( | ||
| "file:// URLs cannot be relative, did you mean to use `.package(path:)`?", diagnosticFile: nil |
There was a problem hiding this comment.
nitpick:
| "file:// URLs cannot be relative, did you mean to use `.package(path:)`?", diagnosticFile: nil | |
| "file:// URLs cannot be relative, did you mean to use '.package(path:)'?", diagnosticFile: nil |
I think it should be single quotes instead of back ticks for consistency.
There was a problem hiding this comment.
#3625 uses backticks, so I kept them as they were. I don't mind changing it if the maintainers agree.
There was a problem hiding this comment.
I think backpacks are probably not correct and we should update everywhere
| """ | ||
| switch expectedError { | ||
| case .relativePath: | ||
| XCTAssertManifestLoadThrows(ManifestParseError.invalidManifestFormat("file:// URLs cannot be relative, did you mean to use `.package(path:)`?", diagnosticFile: nil), stream.bytes) |
There was a problem hiding this comment.
same nitpick as the previous comment:
| XCTAssertManifestLoadThrows(ManifestParseError.invalidManifestFormat("file:// URLs cannot be relative, did you mean to use `.package(path:)`?", diagnosticFile: nil), stream.bytes) | |
| XCTAssertManifestLoadThrows(ManifestParseError.invalidManifestFormat("file:// URLs cannot be relative, did you mean to use '.package(path:)'?", diagnosticFile: nil), stream.bytes) |
|
@karwa @WowbaggersLiquidLunch do you feel this is ready? |
|
@tomerd I didn't look very closely at the code, so I don't really have an opinion. The only thing I reviewed was the back tick vs single quote in diagnostics messages. They've not been updated yet, but my review was just a nitpick, so I think it's fine. |
karwa
force-pushed
the
file-url-hostnames
branch
from
728ca96 to
8aa03c7
Compare
|
@tomerd Yes. I was waiting for the project maintainers' input on @WowbaggersLiquidLunch's suggestion to use single quotes rather than backticks. Since you commented that you agree, I'll take that as confirmation. So I've made that change and rebased the patch. |
|
@swift-ci please smoke test |
|
thanks @karwa |
5 participants
Improve the diagnostics added in #3625 and add tests for more file URLs with hostnames.
CC @neonichu
Motivation:
Previous diagnostic was not entirely clear - the change in the previous patch ensures that all file URLs with hostnames are now rejected. That's a good thing.
localhost- which is non-trivial kind of path that SwiftPM seems to not support right now.This patch doesn't change the functionality from the previous patch; it only makes clear what the limitation is.
Modifications:
Reworded diagnostic, added tests.
Result:
Clearer diagnostic, more tests.