Skip to content

removed OpenGeneratedInterfaceRequest #1532

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 2, 2024
Merged

removed OpenGeneratedInterfaceRequest #1532

merged 1 commit into from
Jul 2, 2024

Conversation

AppAppWorks
Copy link
Contributor

fixed: #1491

@AppAppWorks AppAppWorks requested a review from ahoppen as a code owner June 28, 2024 06:06
ahoppen
ahoppen reviewed Jun 28, 2024
View reviewed changes
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 @AppAppWorks! Great clean-up 👍🏽

Sources/LanguageServerProtocol/Requests/OpenInterfaceRequest.swift Outdated
return self.moduleName
}
}

/// The textual output of a module interface.
public struct GeneratedInterfaceDetails: ResponseType, Hashable {
Copy link
Member

Choose a reason for hiding this comment

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

When we remove the OpenGeneratedInterfaceRequest, we no longer need GeneratedInterfaceDetails in the LanguageServerProtocol module. I think LanguageService.swift would be a better location now.

Sources/LanguageServerProtocol/Requests/OpenInterfaceRequest.swift Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Could you also remove the request from LSP Extensions.md?

Sources/SourceKitLSP/Clang/ClangLanguageService.swift Outdated
@@ -621,8 +621,8 @@ extension ClangLanguageService {
}
return try await forwardRequestToClangd(req)
}

func openGeneratedInterface(_ request: OpenGeneratedInterfaceRequest) async throws -> GeneratedInterfaceDetails? {

Copy link
Member

Choose a reason for hiding this comment

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

Could you run swift-format on your changes? https://github.com/swiftlang/sourcekit-lsp/blob/main/CONTRIBUTING.md#formatting

Sources/SourceKitLSP/LanguageService.swift Outdated
@@ -146,7 +146,12 @@ public protocol LanguageService: AnyObject, Sendable {
func completion(_ req: CompletionRequest) async throws -> CompletionList
func hover(_ req: HoverRequest) async throws -> HoverResponse?
func symbolInfo(_ request: SymbolInfoRequest) async throws -> [SymbolDetails]
func openGeneratedInterface(_ request: OpenGeneratedInterfaceRequest) async throws -> GeneratedInterfaceDetails?
func openGeneratedInterface(
textDocument: TextDocumentIdentifier,
Copy link
Member

Choose a reason for hiding this comment

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

TextDocumentIdentifier is just a wrapper around DocumentURI and DocumentURI is generally easier to work with, so I’d just use DocumentURI here.

Suggested change
textDocument: TextDocumentIdentifier,
document: DocumentURI,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @ahoppen for your review! I've committed the recommended changes accordingly.

@AppAppWorks AppAppWorks force-pushed the remove-OpenGeneratedInterfaceRequest branch from 7dd3e16 to 58e562a Compare June 28, 2024 22:11
ahoppen
ahoppen reviewed Jun 29, 2024
View reviewed changes
Sources/SourceKitLSP/Clang/ClangLanguageService.swift Outdated
Comment on lines 625 to 627
func openGeneratedInterface(document: DocumentURI, moduleName: String, groupName: String?, symbolUSR symbol: String?)
async throws -> GeneratedInterfaceDetails?
{
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: I prefer to wrap the function on the arguments instead of the return type

Suggested change
func openGeneratedInterface(document: DocumentURI, moduleName: String, groupName: String?, symbolUSR symbol: String?)
async throws -> GeneratedInterfaceDetails?
{
func openGeneratedInterface(
document: DocumentURI,
moduleName: String,
groupName: String?,
symbolUSR symbol: String?
) async throws -> GeneratedInterfaceDetails? {

Sources/SourceKitLSP/LanguageService.swift
Comment on lines +161 to +174
func openGeneratedInterface(
document: DocumentURI,
moduleName: String,
groupName: String?,
symbolUSR symbol: String?
) async throws -> GeneratedInterfaceDetails?
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I missed this in the first review. Could you add the comments of the members in OpenGeneratedInterfaceRequest as parameter documentation to this method?

Sources/SourceKitLSP/LanguageService.swift Outdated
@@ -76,6 +76,18 @@ public struct RenameLocation: Sendable {
let usage: Usage
}

/// The textual output of a module interface.
public struct GeneratedInterfaceDetails: ResponseType, Hashable {

Copy link
Member

Choose a reason for hiding this comment

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

Since you’re touching this anyway, could you remove the superfluous newline?

@AppAppWorks
Copy link
Contributor Author

Done! Please have a look :)

ahoppen
ahoppen reviewed Jun 29, 2024
View reviewed changes
Sources/SourceKitLSP/LanguageService.swift Outdated
func openGeneratedInterface(_ request: OpenGeneratedInterfaceRequest) async throws -> GeneratedInterfaceDetails?

/// Request a generated interface of a module to display in the IDE.
/// **(LSP Extension)**
Copy link
Member

Choose a reason for hiding this comment

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

Since this is no longer a request, we don't have to include the LSP Extension text anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! I hope I'll get familiar with the project asap :)

@ahoppen
Copy link
Member

ahoppen commented Jul 1, 2024

Sorry, @AppAppWorks, I introduced a merge conflict with #1534. Could you rebase your changes? Also, could you squash your commits https://github.com/swiftlang/sourcekit-lsp/blob/main/CONTRIBUTING.md#authoring-commits?

@AppAppWorks AppAppWorks force-pushed the remove-OpenGeneratedInterfaceRequest branch from c4d8d20 to bee65ae Compare July 1, 2024 09:18
@AppAppWorks
Copy link
Contributor Author

@ahoppen, rebase done.

@ahoppen
Copy link
Member

ahoppen commented Jul 1, 2024

@swift-ci Please test

@ahoppen ahoppen enabled auto-merge July 1, 2024 09:55
@ahoppen
Copy link
Member

ahoppen commented Jul 1, 2024

@swift-ci Please test Windows

ahoppen
ahoppen reviewed Jul 1, 2024
View reviewed changes
Sources/LanguageServerProtocol/Requests/OpenInterfaceRequest.swift
Copy link
Member

Choose a reason for hiding this comment

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

Could you remove this file from CMakeLists.txt. On Windows SourceKit-LSP is built using CMake instead of SwiftPM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, please check

@AppAppWorks
removed OpenGeneratedInterfaceRequest
eb6f919 
deleted OpenInterfaceRequest.swift and moved GeneratedInterfaceDetails to LanguageService.swift
replaced usages by passing its members as parameters directly, refactored usage of TextDocumentIdentifier to DocumentURI
removed from Messages.builtinRequests
removed from SwiftInterfaceTests
removed from Documentation/`LSP Extensions.md`
removed from Sources/LanguageServerProtocol/CMakeLists.txt
auto-merge was automatically disabled July 1, 2024 22:16

Head branch was pushed to by a user without write access

@AppAppWorks AppAppWorks force-pushed the remove-OpenGeneratedInterfaceRequest branch from bee65ae to eb6f919 Compare July 1, 2024 22:16
@ahoppen
Copy link
Member

ahoppen commented Jul 2, 2024

@swift-ci Please test

@ahoppen ahoppen enabled auto-merge July 2, 2024 04:48
@ahoppen
Copy link
Member

ahoppen commented Jul 2, 2024

@swift-ci Please test Windows

1 similar comment
@ahoppen
Copy link
Member

ahoppen commented Jul 2, 2024

@swift-ci Please test Windows

@ahoppen ahoppen merged commit 91c55f9 into swiftlang:main Jul 2, 2024
3 checks passed
@AppAppWorks AppAppWorks deleted the remove-OpenGeneratedInterfaceRequest branch July 3, 2024 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahoppen ahoppen ahoppen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove OpenGeneratedInterfaceRequest
2 participants
@AppAppWorks @ahoppen