Implement document formatting using swift-format #220
Conversation
|
Very cool! I'll try to review the source changes soon, but first a note about the new dependency:
None of this is a problem; we just need to work through it before we land this PR. |
|
Yeah, there's still a little bit of work we need to do on the swift-format side to get things CI ready, since right now the I've been carving out some time to push a bit more on that recently (e.g., swiftlang/swift-format#136 removes all |
| } | ||
| let options = req.params.options | ||
| self.queue.async { | ||
| var config = SwiftFormatConfiguration.Configuration() |
There was a problem hiding this comment.
When invoked from the LSP, should the formatting action respect the .swift-format.json config file that applies to the file in the request? (Right now, we look in the same directory as the source file and then up through parent directories.)
I'm not sure what the expected behavior is for LSP formatting if you have an external config file and the options passed in the request conflict with those.
But, the SwiftFormatter API doesn't do anything to load that configuration; it's the swift-format command line tool that does it. So if the LSP needs to also look up the configuration, rather than duplicate that file lookup here, we should expose a helper API in SwiftFormatConfiguration that loads the configuration given the path to a particular source file so that other clients can reuse it easily (and so that if we change that logic later, all clients get matching behavior automatically).
There was a problem hiding this comment.
I would expect that we would respect the .swift-format.json configuration in general, but we should have control of this in case we need to mediate some kind of editor-level settings. I think exposing a helper on SwiftFormatConfiguration would be great.
There was a problem hiding this comment.
Maybe we should use the config file if it exists, and fall back on values provided by the lsp if it doesn't exist?
There was a problem hiding this comment.
I changed it so that it tries to load config from file, and if that fails uses values provided by the lsp.
It relies on changes from the swiftlang/swift-format#140 pull request
Trzyipolkostkicukru
force-pushed
the
format
branch
from
9c71595 to
2eaace3
Compare
|
@Trzyipolkostkicukru , @benlangmuir , @allevato any progress here? Bump |
|
I don't know what is needed for swift-format CI integration and haven't tried to find out, so I'm patiently waiting for someone else to do that. No progress from me |
|
No updates from me either, we're blocked by being able to integrate the swift-format dependency. |
|
Sorry for the delays on this, but we've finally managed to carve out time to make some progress here. swiftlang/swift-format#207 is the first PR, and with some follow-up work in the main Swift repository to hook it all together, that should unblock swift-format being used as a dependency of other projects. |
Trzyipolkostkicukru
force-pushed
the
format
branch
from
ed64203 to
36ec508
Compare
|
@benlangmuir I rebased my changes to the master so that they could be merged. Unfortunately doing that broke the tests - it seems |
|
@Trzyipolkostkicukru sorry I missed this comment.
To clarify, is the issue that |
Since |
I am used to waiting weeks and months :D Five days is quick!
Oh god, I seem to have mangled that sentence. I meant to write "and I have to wait for state to stabilize".
The direct issue is that If I remove all tests except one (I cannot filter out the other tests, so I just delete the files for other tests, and comment out the other tests in FormattingTests.swift), then it works more often than not. If I have more than one formatting tests, then it's very rare for more than one to be successful. If I add Potential solution: If I move fetching snapshots into |
Hmm, we should call |
Good to know! I couldn't find any place where
full output |
Hmm it looks like the shutdown method might need to do I think @benlangmuir has a better understanding of the shutdown flow |
|
Yeah, that seems like it ought to work. Or LocalConnection itself should be made thread-safe. |
|
Ping. Any news on this? |
|
The Swift project moved the default branch to More detail about the branch update - https://forums.swift.org/t/updating-branch-names/40412 |
Implements textDocument/formatting request using swift-format library.
It adds a dependency on swift-format which is finicky about which toolchain it uses - it needs to be
swift-DEVELOPMENT-SNAPSHOT-2020-01-29-aor the library will complain at runtime.By the way, is there a better way to get the last Position in a file than loading the contents of the url into a LineTable?