Expose finding ".swift-format" file#140
Conversation
allevato
left a comment
allevato
left a comment
There was a problem hiding this comment.
Thanks! I was going to look into this in a couple days so it saves me some time. Some comments below.
| self.version = highestSupportedConfigurationVersion | ||
| } | ||
|
|
||
| /// Constructs a Configuration using values from a JSON config file |
There was a problem hiding this comment.
Let's leave the format of the file unspecified in the doc comment, so that we have the ability to change it in the future (while still retaining support for older formats):
/// Constructs a Configuration by loading it from a configuration file.
| } | ||
|
|
||
| /// Constructs a Configuration using values from a JSON config file | ||
| public init(configFile url: URL) throws { |
There was a problem hiding this comment.
To match the naming convention of types like Data that load URLs, let's call this init(contentsOf url: URL) throws.
| /// | ||
| /// Looks for a ".swift-format" file in the same directory as the swift file, or its nearest parent. | ||
| /// If one is not found, returns "nil". | ||
| public static func configurationFile(forSwiftFile url: URL) -> URL? { |
There was a problem hiding this comment.
If we're generalizing this so that other people can call it, we don't have to require that the input URL point specifically to a Swift file. For example, we might want to allow someone to pass the URL of a directory to ask "what configuration file applies to files in this directory?"
With that in mind, we could name this func url(forConfigurationFileApplyingTo url: URL) -> URL?, and update the doc comment accordingly, such as
/// Returns the URL of the configuration file that applies to the given file or directory.
I think we can be vague about the exact search behavior in the doc comment because it will give us the flexibility to adjust it in the future as well.
But we'll also need to update the implementation below: before the loop, we'll need to check to see if the incoming URL is a file (rather than a directory), and if it is, delete the file component from the path. Then we'll also need to adjust the first line of the loop and the condition slightly to make sure that we still go up to the parent at the right time and exit correctly when we hit the root.
There was a problem hiding this comment.
I added a placeholder component for files instead of adjusting the loop condition, because otherwise it would be hard to write it in a way that doesn't skip the root directory
| return testPath | ||
| } | ||
| } while path.path != "/" | ||
| if let swiftFileUrl = swiftFilePath.map(URL.init(fileURLWithPath:)), |
There was a problem hiding this comment.
Please use swiftFileURL and configFileURL throughout.
Update documentation to make it more future-proof Make `url(forConfigurationFileApplyingTo:)` able to handle directories Use better names
allevato
left a comment
allevato
left a comment
There was a problem hiding this comment.
Looks good!
I'm fine with no tests for this right now because the best way to test it would be to replace all of the existing FileManager usage with the FileSystem abstraction from swift-tools-support-core so that we can mock out the FS contents for tests, and that's a lot to try to do in this PR.
* add import * linux only fix * update packages * update to GraphViz 0.1.2 * update Package.resolved * Add Changelog entry Co-authored-by: Mattt <mattt@me.com>
2 participants
Expose finding ".swift-format" file, and creating Configuration with that file, so that it could be used with sourcekit-lsp
I hope it's okay if there are no tests for this functionality in here, and instead it's tested indirectly by the sourcekit-lsp with an awesome Tibs that makes testing multi file projects easy