appendingPathComponent(:isDirectory:) should account for isDirectory #23926
Conversation
| @@ -827,7 +827,8 @@ public struct URL : ReferenceConvertible, Equatable { | |||
| } else { | |||
| // Now we need to do something more expensive | |||
| if var c = URLComponents(url: self, resolvingAgainstBaseURL: true) { | |||
| c.path = (c.path as NSString).appendingPathComponent(pathComponent) | |||
| let path = (c.path as NSString).appendingPathComponent(pathComponent) | |||
| c.path = isDirectory ? path + "/" : path | |||
There was a problem hiding this comment.
Should we check if a path separator exists here at the end and not double-up?
There was a problem hiding this comment.
No need to check the existence of a separator to mimic the behavior of NSURL.appendingPathComponent(:isDirectory:): it will always append a separator.
|
cc @compnerd does NSString add forward slashes here? It only converts to Windows paths when getting the FSR, right? |
|
@millenomi - yeah, adding the forward slash is fine, the FSR will normalise the path separators. |
|
@swift-ci please test and merge |
|
We'll want an equivalent patch for apple/swift-corelibs-foundation, I assume, but I haven't checked if we have equivalent code there. |
|
@millenomi oh you're right, same issue at https://github.com/apple/swift-corelibs-foundation/blob/aec0d6f7b24e3fdf6790956c4f6570a1105091bb/Foundation/URL.swift#L695 I've PR it at swiftlang/swift-corelibs-foundation#2085. |
|
@millenomi can you run the merge again? CI didn't pass for some reason. |
|
@swift-ci please test and merge |
|
@swift-ci test |
|
Build failed |
This comment has been minimized.
This comment has been minimized.
|
@swift-ci clean test macOS |
|
@Coeur Test and merge check is used to trigger other required tests. You dont need to have "Test and Merge" passing, anyone with commit access should be able to merge this PR now because it passed required testing. |
|
Since it’s been a few days, @swift-ci please smoke test |
|
@millenomi @CodaFi can we get it merged? Thank you. |
|
It's up to Foundation. |
|
rdar://61796888 |
|
@millenomi Looks like you’d approved but merge bots failed. Still OK to merge? |
|
Let's try again. |
|
@swift-ci please test and merge |
|
If there is a test failure, we can examine it and go from there. |
|
@swift-ci please test |
When
appendingPathComponent(:isDirectory:)is falling back from an NSURL.appendingPathComponent() to an NSString.appendingPathComponent(), it's forgetting to take into account theisDirectoryparameter.