Implemented FileManager.delegate and cross-device move #1673
Conversation
|
@parkera would you mind reviewing this PR? |
Foundation/FileManager.swift
Outdated
| } | ||
|
|
||
| @inline(__always) | ||
| func shouldMoveItemAtPath(_ path: String, toPath: String, isURL: Bool) -> Bool { |
There was a problem hiding this comment.
Should these be fileprivate as well?
There was a problem hiding this comment.
This could probably be private since it is in the class and the @inline(__always) probably isn't required as the compiler can make that decision.
Foundation/FileManager.swift
Outdated
| // TODO: Copy and delete. | ||
| NSUnimplemented("Cross-device moves not yet implemented") | ||
| try _copyOrLinkDirectoryHelper(atPath: srcPath, toPath: dstPath) { (srcPath, dstPath, fileType) in | ||
| guard shouldMoveItemAtPath(srcPath, toPath: dstPath, isURL: isURL) else { |
There was a problem hiding this comment.
This is redundant with the initial shouldMoveItemAtPath check, no?
There was a problem hiding this comment.
No, for two reason:
- This body is called only when
rename()fails - This is called for every file for recursive copy/move
There was a problem hiding this comment.
FWIW, Darwin's implementation doesn't ask "should move" for every subitem when it falls back to a copy. The delegate-calling semantics of the operation is identical in either case. As such, the initial shouldMove check should be sufficient confirmation for either the rename or the recursive copy.
There was a problem hiding this comment.
Thanks, I will chack again 🙏
Foundation/FileManager.swift
Outdated
| while let current = fts_read(stream)?.pointee { | ||
|
|
||
|
|
||
| let itemPath = NSString(bytes: current.fts_path, |
There was a problem hiding this comment.
@spevans Can I replace this with String.init(cString:)?
There was a problem hiding this comment.
Best to use self.string(withFileSystemRepresentation:length:) to convert to a String.
|
Changes:
|
|
@swift-ci test |
1 similar comment
|
@swift-ci test |
|
Hi, is it normal to have a pull request open without and feedback/activity? @parkera |
|
Apologies, that's my fault. I dropped the ball on following up after my previous feedback. I'll take another look at this today. |
|
@kperryua pardon me for disturbing. But it’s hard to keep unmerged PRs synchronized. |
|
@parkera pardon me for disturbing you repeatedly. Would you mind review this PR? |
|
If @kperryua approves that's as good as my approval. |
|
They approved as I see. |
|
@swift-ci test and merge |
FileManager.delegatefunctionalityFileManagerDelegatedefault implementation method signature to match protocol methods.moveItem()removeItem()