-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[Diagnostics] Use imperative msg for protocol conformance & switch-case fixits #67909
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah given the discussion in the issue I think moving to imperative form is good
@swift-ci please smoke test |
…se fixits This commit changes fixit messages from a question/suggestion to an imperative message for protocol conformances and switch-case. Addresses swiftlang#67510.
9f3b224
to
8e2e625
Compare
Previous CI run had some failing tests - addressed them with a new commit. @ktoso |
Thanks, let's give this a full run to be sure that's everything covered |
@swift-ci please test |
@ktoso - Linux build is failing because of this - https://github.com/apple/sourcekit-lsp/blob/6d3db6d0850d53d9521374c1c18212d6834ae1f9/Tests/SourceKitLSPTests/PullDiagnosticsTests.swift#L121. I can open a PR for that repo in parallel. |
And the macOS build is failing on |
@swift-ci please test |
The windows had succeeded earlier but timed out this time 😞 I think Linux and macOS builds are expected to fail because they are running tests that are in a different repo - https://github.com/apple/sourcekit-lsp/blob/6d3db6d0850d53d9521374c1c18212d6834ae1f9/Tests/SourceKitLSPTests/PullDiagnosticsTests.swift#L121. What is the best way to address this? One way is to create a parallel PR that changes the |
That is indeed the way right now 😅. We can do cross-PR testing here to check the combined PRs. |
This commit corresponds to changes introduced in swiftlang/swift#67909.
How can that be done? I created swiftlang/sourcekit-lsp#799 in parallel but both need to be built together. Another way is to avoid running the |
@swift-ci please test |
Can the windows build be re-triggered? All other builds succeeded. |
@swift-ci please test Windows platform |
@bnbarham Thanks for triggering the CI. The builds succeeded - please merge both the PRs when you get a chance, thanks! |
Thank you for your contribution @nishithshah2211 |
Thanks for the merge! |
The Fix-It message that we are testing here has changed in swiftlang/swift#67909. Accept both the old and the new message to make sure SourceKit-LSP tests also pass with older toolchains (e.g. if are using Xcode 15.0, which doesn’t contain the change yet). rdar://116706363
The Fix-It message that we are testing here has changed in swiftlang/swift#67909. Accept both the old and the new message to make sure SourceKit-LSP tests also pass with older toolchains (e.g. if are using Xcode 15.0, which doesn’t contain the change yet).
The Fix-It message that we are testing here has changed in swiftlang/swift#67909. Accept both the old and the new message to make sure SourceKit-LSP tests also pass with older toolchains (e.g. if are using Xcode 15.0, which doesn’t contain the change yet). rdar://116706363
This commit changes fixit messages from a question/suggestion to an imperative message for protocol conformances and switch-case. The only source change is in
DiagnosticsSema.def
. Rest of the changes are in tests to get them to pass.Resolves #67510.