Improve path management and filesystem operation ergonomics #318
Conversation
Use FilePath instead of file URL's. FilePath is recommended as the system data type to be used to represent local file paths for command-line tools. The methods have simpler names while retaining the same vital path arithmetic functions. It is much less likely that a stringer will accidentally print out a file URL to the user when a path is intended. Remove usage of 'FileManager.default' in favour of API's that are far less verbose to type and read. Make top-level API functions for operations, such as checking if a file exists, removing files, moving them, copying them, etc. Once SwiftlyCore is imported then these functions become available for use. These functions accept FilePath, not URL or String for a measure of type safety. Make the new API's async by default to permit swapping FileManager with another implementation that has async operations. The most common file path operation is appending. Make use of operator overloading to make these operations much cleaner, and clearer with the division operator.
|
@swift-ci test macOS |
|
@swift-ci test macOS |
|
@swift-ci test macOS |
|
@swift-ci test macOS |
| } | ||
|
|
||
| extension FilePath { | ||
| public static func / (left: FilePath, right: String) -> FilePath { |
There was a problem hiding this comment.
I don't love this kind of pseudo operator overloading. To me it obscures the intent vs plainly calling left.append(right).
In general Swift maintains that an overloaded operator should be consistent with the intent, so overloading + to work with a Point type makes sense since adding together the Point's coordinates is a natural operation consistent with the intent of addition. This meant that on first read I interpreted these usages as "divide the one segment in to the path into the other" before double taking and realizing that doesn't make sense.
There was a problem hiding this comment.
There's a tradeoff here in this case. I think that like with Python's pathlib package, people are rarely expecting that there's some kind of mathematical divide happening between file paths as they might if the data structure was more mathematical in nature. With path operations, there's a clear benefit to being able to see the paths, more than the method names, and Swift types involved so that one can reason about them at a higher level.
mkdir( [[ somePath / "usr/bin/foo" ]])
vs.
FileManager.default.createDirectory( [[ somePath ]] .appending( [["usr/bin/foo" ]]))
It's easier to piece together what the complete path will be in the first example with fewer words that punctuate it.
There was a problem hiding this comment.
Fair, its a valid trade off, I've been bitten by overloaded operators before but in this case its probably pretty benign.
| try FileManager.default.createSymbolicLink(atPath: atPath, withDestinationPath: linkPath) | ||
| } | ||
|
|
||
| public func chmod(atPath: FilePath, mode: Int) async throws { |
There was a problem hiding this comment.
Is there a reason these are defined as free functions instead of extensions on FileManager? Seems like some vars like homeDir are implemented in two places.
There was a problem hiding this comment.
This is related to ergonomics. Having to type FileManager.default.someLongFunctionName() every time, and also to read that makes parts of swiftly that are script-like, and even actual scripts like build-swiftly-release, much harder to reason about when it's the filesystem operations and paths that are the truly important parts. The free functions make it much more streamlined for these situations.
There was a problem hiding this comment.
Generally I persist a local var like let fm = FileManager.default to make these calls more concise (i.e. fm.chmod(...)). This also lets you more easily swap out the FileManager implementation in tests to, say, an in memory file system. See something like FileSystem in tools-support-core.
There was a problem hiding this comment.
This could also be a type alias so that the entire module (and dependent modules) can use 'fs.xyz()' too. I'll try this out.
| @@ -56,31 +57,31 @@ public struct RunProgramError: Swift.Error { | |||
| public protocol Platform: Sendable { | |||
| /// The platform-specific default location on disk for swiftly's home | |||
| /// directory. | |||
| var defaultSwiftlyHomeDirectory: URL { get } | |||
| var defaultSwiftlyHomeDir: FilePath { get } | |||
There was a problem hiding this comment.
praise: nice consistency improvement
Add typealias to fs for Filesystem in dependent modules to shorten fs calls
|
@swift-ci test macOS |
|
@swift-ci test macOS |
Use FilePath instead of file URL's. FilePath is recommended as the system data type to be used to represent local file paths for command-line tools. The methods have simpler names while retaining the same vital path arithmetic functions. It is much less likely that a stringer will accidentally print out a file URL to the user when a path is intended.
Remove usage of 'FileManager.default' in favour of API's that are far less verbose to type and read. Make shorter and more command-line recognizable FileSystem API functions for operations, such as checking if a file exists, removing files, moving them, copying them, etc. These functions accept FilePath, not URL or String for a measure of type safety. Make the new API's async by default to permit swapping FileManager with another implementation that has async operations.
The most common file path operation is appending. Make use of operator overloading to make these operations much cleaner, and clearer with the division operator.