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.

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

Add textDocument/doccDocumentation request #1851

Merged
merged 27 commits into from
Jan 20, 2025
Merged

Add textDocument/doccDocumentation request #1851

merged 27 commits into from
Jan 20, 2025

Conversation

matthewbastien
Copy link
Member

@matthewbastien matthewbastien commented Dec 4, 2024 •
edited
Loading

Support documentation live preview in Swift files by handling requests to convert documentation using Swift DocC.

This PR adds a new request method textDocument/doccDocumentation that responds with a JSON encoded RenderNode created using Swift DocC's ConvertService. This request has two parameters:

  1. A text document for which to convert documentation
  2. An optional position within the text document that will be used to find the nearest documentable symbol

The request is only supported on macOS and Linux until Swift DocC can be made to support Windows.

@matthewbastien matthewbastien force-pushed the convert-documentation branch 2 times, most recently from 3992c85 to 55b492d Compare December 6, 2024 15:24
@matthewbastien matthewbastien force-pushed the convert-documentation branch 2 times, most recently from 88e5500 to f4e4e06 Compare December 16, 2024 20:25
@matthewbastien matthewbastien marked this pull request as ready for review December 19, 2024 15:41
@matthewbastien
Copy link
Member Author

@swift-ci please test

2 similar comments
@matthewbastien
Copy link
Member Author

@swift-ci please test

@matthewbastien
Copy link
Member Author

@swift-ci please test

@matthewbastien
Copy link
Member Author

@swift-ci please test windows platform

@matthewbastien
Copy link
Member Author

@swift-ci please test

@matthewbastien
Copy link
Member Author

@swift-ci please test windows platform

@matthewbastien
Copy link
Member Author

@swift-ci please test

ahoppen
ahoppen reviewed Jan 8, 2025
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 for implementing this @matthewbastien! Looking forward to seeing docc-support in VS Code.

Contributor Documentation/LSP Extensions.md Outdated Show resolved Hide resolved
Contributor Documentation/LSP Extensions.md Outdated Show resolved Hide resolved
Sources/LanguageServerProtocol/Messages.swift Outdated Show resolved Hide resolved
Sources/LanguageServerProtocol/Requests/ConvertDocumentationRequest.swift Outdated Show resolved Hide resolved
Contributor Documentation/LSP Extensions.md Outdated Show resolved Hide resolved
Sources/SourceKitLSP/SourceKitLSPServer.swift Outdated Show resolved Hide resolved
Tests/SourceKitLSPTests/ConvertDocumentationTests.swift Outdated Show resolved Hide resolved
Tests/SourceKitLSPTests/ConvertDocumentationTests.swift Outdated Show resolved Hide resolved
Tests/SourceKitLSPTests/ConvertDocumentationTests.swift Outdated Show resolved Hide resolved
Tests/SourceKitLSPTests/ConvertDocumentationTests.swift Outdated Show resolved Hide resolved
@matthewbastien
Copy link
Member Author

@swift-ci please test

@matthewbastien
Copy link
Member Author

@swift-ci please test windows platform

@matthewbastien matthewbastien changed the title Add textDocument/convertDocumentation request Add textDocument/doccDocumentation request Jan 15, 2025
@matthewbastien
Copy link
Member Author

@swift-ci please test

@matthewbastien
Copy link
Member Author

@swift-ci please test windows platform

@matthewbastien
Copy link
Member Author

@swift-ci please test

@matthewbastien
Copy link
Member Author

@swift-ci please test windows platform

@matthewbastien
Copy link
Member Author

@swift-ci please test

@matthewbastien
Copy link
Member Author

@swift-ci please test windows platform

ahoppen
ahoppen reviewed Jan 16, 2025
View reviewed changes
Contributor Documentation/LSP Extensions.md Outdated Show resolved Hide resolved
Contributor Documentation/LSP Extensions.md Outdated Show resolved Hide resolved
Sources/LanguageServerProtocol/Requests/DoccDocumentationRequest.swift Outdated Show resolved Hide resolved
Sources/SKTestSupport/TestSourceKitLSPClient.swift Outdated Show resolved Hide resolved
Sources/SourceKitLSP/Documentation/DocCServer.swift Outdated Show resolved Hide resolved
Sources/SourceKitLSP/Documentation/DocumentationManager.swift Outdated Show resolved Hide resolved
Sources/SourceKitLSP/Documentation/DocumentationManager.swift
Comment on lines +116 to +118
guard let renderNodeData = convertResponse.renderNodes.first else {
throw ResponseError.internalError("SwiftDocC did not return any render nodes")
}
Copy link
Member

Choose a reason for hiding this comment

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

I don’t know much about docc but when would the response contain multiple render nodes? And would it be safe to ignore any but the first in that case?

Copy link
Member Author

@matthewbastien matthewbastien Jan 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

DocC will only emit multiple render nodes if you request multiple USRs to convert. In our case we only ever request documentation for a single USR.

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 add that as a comment so we don’t have to wonder about it again?

Sources/SourceKitLSP/Documentation/DocumentationManager.swift Outdated Show resolved Hide resolved
Sources/SourceKitLSP/Documentation/DocumentationManager.swift Outdated Show resolved Hide resolved
Sources/LanguageServerProtocol/Messages.swift Outdated Show resolved Hide resolved
@matthewbastien
fix failure message in test
f5a57bc
@matthewbastien
minor code quality improvements
6ed5cbe
@matthewbastien
fix build errors
4606232
@matthewbastien
revert changes to TestSourceKitLSPClient
af02df9
@matthewbastien
fix documentation comments for enum case statements
2ba6fa0
@matthewbastien
fix test compilation failure
ca9d2e9
@matthewbastien
rename request to textDocument/doccDocumentation
f03ae0b
@matthewbastien
fix default dependency type for TextDocumentRequests
3c318ec
@matthewbastien
make position optional
0a95899
@matthewbastien
use built-in LSP ResponseError with code RequestFailed for errors
5c31366
@matthewbastien
add encoding and decoding tests
bf8e86f
@matthewbastien
prefer async over completion handlers
4d0d277
@matthewbastien
add documentation for DocCServer.convert
f1ffeaf
@matthewbastien
use default QoS for the DocCServer
b48b197
@matthewbastien
use BuildSystemManager.moduleName(for:in:) instead of BuildTarget.dis…
74cb30e 
…playName
@matthewbastien
simplify DocumentableSymbolFinder
17ad9b4
@matthewbastien
clean up request forwarding
4b9288a
@matthewbastien
use dictionary in tests rather than array
b4ba5f5
@matthewbastien
start test markers at 1️⃣
cb66e17
@matthewbastien
more test cleanup
d1e73f5
@matthewbastien
add canImport check around SourceKitLSPServer.doccDocumentation(_:)
f6bec67
@matthewbastien
address review comments
6675d27
@matthewbastien
clean up unnecessary commas
d0a10de
@matthewbastien
simplify DocumentableSymbol search
561aad2
@matthewbastien
Copy link
Member Author

@swift-ci please test

@matthewbastien
Copy link
Member Author

@swift-ci please test windows platform

ahoppen
ahoppen approved these changes Jan 20, 2025
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.

Great work. Let’s get this in.

@ahoppen ahoppen merged commit aeb6c14 into swiftlang:main Jan 20, 2025
3 checks passed
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.
2 participants
@matthewbastien @ahoppen