preserve trailing slash in uri path#107
Conversation
| pathEndIndex = url.index(before: fragmentIndex) | ||
| } | ||
|
|
||
| return url[pathEndIndex] == "/" |
There was a problem hiding this comment.
Writing this against the absolute string seems to be making life quite hard: why not write it against self.path?
There was a problem hiding this comment.
self.path removes trailing slash
Lukasa
left a comment
Lukasa
left a comment
There was a problem hiding this comment.
Can we step back for a minute: why is the current behaviour of URL problematic today?
|
If a server has a redirect like |
|
It's a shame that server is broken. |
| pathEndIndex = url.index(before: queryIndex) | ||
| } else if let fragmentIndex = url.suffix(from: url.firstIndex(of: "@") ?? url.startIndex).lastIndex(of: "#") { | ||
| pathEndIndex = url.index(before: fragmentIndex) | ||
| } |
There was a problem hiding this comment.
Do we need the else branch? The minimum versions above were all released in 2015, making them 4 years old this year. Can we get away without it?
If the answer is really no, then can we also add a comment to this code branch to say that the code is not 100% correct, but is considered "good enough"?
There was a problem hiding this comment.
What do you mean by getting away without it? Adding supported platforms in Package.swift or a precondition? I would prefer not to have an alternative clause here as well...
@tomerd @tanner0101 @ianpartridge do you think we should limit the client to modern(er) platforms?
There was a problem hiding this comment.
given our main use case is linux, restricting older non-linux platforms in Package.swift seems fine to me
|
@Lukasa fixed path check by using |
|
|
||
| let package = Package( | ||
| name: "async-http-client", | ||
| platforms: [.macOS(.v10_11), .iOS(.v9), .tvOS(.v9), .watchOS(.v2)], |
There was a problem hiding this comment.
would this potentially confuse folks to think linux is not supported? maybe worth adding a comment?
There was a problem hiding this comment.
added a comment!
There was a problem hiding this comment.
I think we need to take this out. If we don't then every dependency needs to mirror this stanza.
weissi
left a comment
weissi
left a comment
There was a problem hiding this comment.
Sorry, the patch itself looks good but the platform restriction is probably too annoying.
|
|
||
| let package = Package( | ||
| name: "async-http-client", | ||
| platforms: [.macOS(.v10_11), .iOS(.v9), .tvOS(.v9), .watchOS(.v2)], |
There was a problem hiding this comment.
I think we need to take this out. If we don't then every dependency needs to mirror this stanza.
|
Agreed, reverted |
4 participants
fixes #101 by preserving trailing slash in path return by the
URL'spathproperty