Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle on-type formatting requests #1815

Merged
merged 1 commit into from
Nov 22, 2024

Conversation

MahdiBM
Copy link
Contributor

@MahdiBM MahdiBM commented Nov 10, 2024

To fully resolve #1805

TODO

  • Hide behind an experimental flag
  • Make sure the experimental flag is enable-able in VSCode?

@MahdiBM MahdiBM requested a review from ahoppen as a code owner November 10, 2024 15:19
@MahdiBM MahdiBM marked this pull request as draft November 10, 2024 15:23
@MahdiBM MahdiBM force-pushed the mmbm-on-type-formatting branch 2 times, most recently from 4928cd4 to e21bf00 Compare November 10, 2024 15:38
Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on the implementation. I’m still a little skeptical how well this will work in practice, but I’m happy to iterate on it behind an experimental flag. If we do get it working, that would be great.

Make sure the experimental flag is enable-able in VSCode?

You should be able to enable the experimental feature in your Configuration File. There’s an option for that.

Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good to me once they are guarded behind an experimental feature.

@MahdiBM
Copy link
Contributor Author

MahdiBM commented Nov 12, 2024

I'll handle the experimental-feature gating later 🙂.

@MahdiBM MahdiBM force-pushed the mmbm-on-type-formatting branch from a79973b to 36e6feb Compare November 16, 2024 15:15
@MahdiBM MahdiBM requested a review from ahoppen November 16, 2024 15:26
@MahdiBM MahdiBM marked this pull request as ready for review November 16, 2024 15:26
@MahdiBM MahdiBM force-pushed the mmbm-on-type-formatting branch from 96852b4 to 66b8400 Compare November 16, 2024 15:29
Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. Just one comment on a comment 😉

@MahdiBM MahdiBM force-pushed the mmbm-on-type-formatting branch from cf43ffe to 719dd87 Compare November 16, 2024 17:22
@MahdiBM MahdiBM requested a review from ahoppen November 16, 2024 17:22
@MahdiBM MahdiBM force-pushed the mmbm-on-type-formatting branch from da711b4 to b1b7b86 Compare November 16, 2024 17:24
Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thank you @MahdiBM

@ahoppen ahoppen enabled auto-merge November 16, 2024 17:28
@ahoppen
Copy link
Member

ahoppen commented Nov 16, 2024

@swift-ci Please test

@ahoppen
Copy link
Member

ahoppen commented Nov 16, 2024

@swift-ci Please test Windows

auto-merge was automatically disabled November 17, 2024 16:23

Head branch was pushed to by a user without write access

@MahdiBM MahdiBM force-pushed the mmbm-on-type-formatting branch from 4bba480 to 5adec10 Compare November 17, 2024 16:24
@MahdiBM
Copy link
Contributor Author

MahdiBM commented Nov 17, 2024

Fixed this by adding the import:

159 |   package func documentOnTypeFormatting(_ req: DocumentOnTypeFormattingRequest) async throws -> [TextEdit]? {
160 |     let snapshot = try documentManager.latestSnapshot(req.textDocument.uri)
161 |     guard let line = snapshot.lineTable.line(at: req.position.line) else {
    |                                         `- error: instance method 'line(at:callerFile:callerLine:)' is not available due to missing import of defining module 'SKUtilities'
162 |       return nil
163 |     }

@MahdiBM MahdiBM requested a review from ahoppen November 17, 2024 16:26
@ahoppen
Copy link
Member

ahoppen commented Nov 19, 2024

Thanks for the fix.

@swift-ci Please test

@ahoppen ahoppen enabled auto-merge November 19, 2024 00:13
auto-merge was automatically disabled November 19, 2024 18:04

Head branch was pushed to by a user without write access

@MahdiBM MahdiBM requested a review from ahoppen November 19, 2024 18:10
@ahoppen
Copy link
Member

ahoppen commented Nov 19, 2024

Could you rebase your PR instead of merging main into your branch? It makes for a less convoluted git history.

@MahdiBM MahdiBM force-pushed the mmbm-on-type-formatting branch 2 times, most recently from 3bbc6c4 to b001978 Compare November 19, 2024 19:15
@MahdiBM
Copy link
Contributor Author

MahdiBM commented Nov 19, 2024

@ahoppen done

@ahoppen
Copy link
Member

ahoppen commented Nov 19, 2024

@swift-ci Please test

@ahoppen
Copy link
Member

ahoppen commented Nov 19, 2024

@swift-ci Please test Windows

@MahdiBM MahdiBM force-pushed the mmbm-on-type-formatting branch from b001978 to 2290a65 Compare November 21, 2024 12:32
@MahdiBM MahdiBM force-pushed the mmbm-on-type-formatting branch from b597a78 to a2eb7b9 Compare November 21, 2024 12:37
@MahdiBM
Copy link
Contributor Author

MahdiBM commented Nov 21, 2024

@ahoppen rebased with main again, this was the error which looks like unrelated to this PR?

sourcekit-lsp/Sources/BuildSystemIntegration/SwiftPMBuildSystem.swift:318:107: error: cannot convert value of type 'URL' to expected argument type 'TSCAbsolutePath' (aka 'AbsolutePath')
316 |       )
317 |     } else if let scratchDirectory = options.swiftPMOrDefault.scratchPath,
318 |       let scratchDirectoryPath = try? AbsolutePath(validating: scratchDirectory, relativeTo: AbsolutePath(projectRoot))
    |                                                                                                           `- error: cannot convert value of type 'URL' to expected argument type 'TSCAbsolutePath' (aka 'AbsolutePath')
319 |     {
320 |       location.scratchDirectory = scratchDirectoryPath

/home/build-user/sourcekit-lsp/Sources/BuildSystemIntegration/SwiftPMBuildSystem.swift:479:14: warning: When we drop support for Swift 5.10 we no longer need to adjust compiler arguments for the Modules move
477 | 
478 |     #if compiler(>=6.1)
479 |     #warning("When we drop support for Swift 5.10 we no longer need to adjust compiler arguments for the Modules move")
    |              `- warning: When we drop support for Swift 5.10 we no longer need to adjust compiler arguments for the Modules move
480 |     #endif
481 |     // Fix up compiler arguments that point to a `/Modules` subdirectory if the Swift version in the toolchain is less

@ahoppen
Copy link
Member

ahoppen commented Nov 21, 2024

Ah, that was because of a merge conflict that I resolved in #1838

@ahoppen
Copy link
Member

ahoppen commented Nov 21, 2024

@swift-ci Please test

@ahoppen ahoppen enabled auto-merge November 21, 2024 17:15
@ahoppen
Copy link
Member

ahoppen commented Nov 21, 2024

@swift-ci Please test Windows

1 similar comment
@ahoppen
Copy link
Member

ahoppen commented Nov 22, 2024

@swift-ci Please test Windows

@ahoppen ahoppen merged commit 3b6ced8 into swiftlang:main Nov 22, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle "textDocument/rangeFormatting" and "textDocument/onTypeFormatting"
2 participants