Support formatting of entire documents #769
Conversation
|
@swift-ci Please test |
|
CC @allevato, I’d appreciate your review of how I’m calling swift-format’s API. |
| @@ -0,0 +1,4 @@ | |||
| This file is intentionally an invalid swift-format configuration file. It is used to test | |||
| the behavior when program cannot find a valid file. We cannot just left this directory blank, | |||
| // Unsupported options | ||
| if req.options.trimTrailingWhitespace == false { | ||
| throw ResponseError(code: .requestFailed, message: "swift-format does not support keeping trailing whitespace; set format option to trim trailing trivia to run the formatter") | ||
| } | ||
| if req.options.insertFinalNewline == false { | ||
| throw ResponseError(code: .requestFailed, message: "swift-format always inserts a final newline to the file; set option to insert a final newline to run the formatter") | ||
| } | ||
| if req.options.trimFinalNewlines == false { | ||
| throw ResponseError(code: .requestFailed, message: "swift-format always trims final newlines; set option to trim final newlines to run the formatter") | ||
| } |
There was a problem hiding this comment.
These messages are all a little confusing to me, especially the first. Is the ; set option... really needed? If it is, can we name it explicitly with the actual value (true)?
|
@swift-ci Please test Windows |
|
@swift-ci test |
|
Putting on hold until we can build swift-format on Windows. |
Is there already an issue for this in swift-format? Doing a cursory search there I only found https://github.com/apple/swift-format/issues?q=is%3Aissue+is%3Aopen+windows |
|
No, but I think it's on @compnerd mental to do list |
ahoppen
force-pushed
the
ahoppen/document-formatting
branch
from
0baa53d to
4916151
Compare
|
I changed the implementation to invoke swift-format as an executable instead of having SourceKit-LSP link against SwiftFormat. |
|
@swift-ci Please test |
With swiftlang/sourcekit-lsp#769, sourcekit-lsp supports formatting that invokes swift-format from the toolchain and tests that test this behavior. For those to pass, we need to install swift-format into the toolchain on all jobs that test sourcekit-lsp.
|
@swift-ci Please test |
With swiftlang/sourcekit-lsp#769, sourcekit-lsp supports formatting that invokes swift-format from the toolchain and tests that test this behavior. For those to pass, we need to install swift-format into the toolchain on all jobs that test sourcekit-lsp.
|
@swift-ci Please test |
README.md
Outdated
| @@ -46,7 +46,7 @@ SourceKit-LSP is still in early development, so you may run into rough edges wit | |||
| | Workspace Symbols | ✅ | | | |||
| | Rename | ❌ | | | |||
| | Local Refactoring | ✅ | | | |||
| | Formatting | ❌ | | | |||
| | Formatting | ✅ | Whole file at once only. | | |||
There was a problem hiding this comment.
| | Formatting | ✅ | Whole file at once only. | | |
| | Formatting | ✅ | Whole file only | |
| // Most likely, the editor inferred them from the current document and thus the options | ||
| // passed by the editor are most likely less correct than those in .swift-format. | ||
| return try await body(configFile) | ||
| } else { |
There was a problem hiding this comment.
I'd prefer no else. Does swift-format allow setting up options without a file so there would be no need for the temporary file?
There was a problem hiding this comment.
We added the ability to pass a raw JSON string in the --configuration flag a while back: swiftlang/swift-format#634
There was a problem hiding this comment.
Oh, nice. I didn’t know that. That makes the implementation a lot nicer. 😍
| } | ||
|
|
||
| let formattedBytes = try await withSwiftFormatConfiguration(for: req.textDocument.uri, options: req.options) { | ||
| (formatConfigUrl) -> [UInt8] in |
There was a problem hiding this comment.
Do we have a bug open in swift-format for this? The formatting for closures makes very little sense IMO. It would be better to make the for:options: on newlines and then keep the arguments with the { (which could either be on a newline, or with the last argument to the call).
There was a problem hiding this comment.
Depend on the swift-format library to discover and write the swift-format configuration file. Invoke swift-format from the toolchain to actually format a document. This makes sure that the formatting of SourceKit-LSP and the swift-format executable in the toolchain never get out of sync. Fixes swiftlang#576 rdar://96159694
ahoppen
force-pushed
the
ahoppen/document-formatting
branch
from
4916151 to
79cc4f9
Compare
|
@swift-ci Please test |
|
@swift-ci Please test Windows |
With swiftlang/sourcekit-lsp#769, sourcekit-lsp supports formatting that invokes swift-format from the toolchain and tests that test this behavior. For those to pass, we need to install swift-format into the toolchain on all jobs that test sourcekit-lsp.
Depend on the swift-format library to discover and write the swift-format configuration file. Invoke swift-format from the toolchain to actually format a document. This makes sure that the formatting of SourceKit-LSP and the swift-format executable in the toolchain never get out of sync.
Fixes #576
rdar://96159694