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

Provide links to locally built documentation for experimental/externalDocs #14662

Conversation

Ddystopia
Copy link
Contributor

@Ddystopia Ddystopia commented Apr 26, 2023

This pull request addresses issue #12867, which requested the ability to provide links to locally built documentation when using the "Open docs for symbol" feature. Previously, rust-analyzer always used docs.rs for this purpose. With these changes, the feature will provide both web (docs.rs) and local documentation links without verifying their existence.

Changes in this PR:

  • Added support for local documentation links alongside web documentation links.
  • Added target_dir path argument for external_docs and other related methods.
  • Added sysroot argument for external_docs.
  • Added target_directory path to CargoWorkspace.

API Changes:

  • Added an experimental client capability { "localDocs": boolean }. If this capability is set, the Open External Documentation request returned from the server will include both web and local documentation links in the ExternalDocsResponse object.

Here's the ExternalDocsResponse interface:

interface ExternalDocsResponse {
    web?: string;
    local?: string;
}

By providing links to both web-based and locally built documentation, this update improves the developer experience for those using different versions of crates, git dependencies, or local crates not available on docs.rs. Rust-analyzer will now provide both web (docs.rs) and local documentation links, leaving it to the client to open the desired link. Please note that this update does not perform any checks to ensure the validity of the provided links.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 26, 2023
@nemethf
Copy link
Contributor

nemethf commented Apr 26, 2023

Can this be implemented in a backward compatible way? I.e., clients conforming to the current extension might fail in the future because the type of the return value has changed. (One possibility to overcome this issue is make rust-analyzer check for a client capability like, for example, "externalDocs": {"version": 2}.)

Also, does the local documentation count as "external documentation"?

Thank you.

@Ddystopia
Copy link
Contributor Author

Ddystopia commented Apr 26, 2023

Can this be implemented in a backward compatible way? I.e., clients conforming to the current extension might fail in the future because the type of the return value has changed. (One possibility to overcome this issue is make rust-analyzer check for a client capability like, for example, "externalDocs": {"version": 2}.)

As far as I know, rust-analyzer does not provide a stable API interface, so breakage is fine.
Also, there is #4604 for this purpose. As stated:

This issue exists to announce changes which might affect rust-analyzer plugins for different editors.
If you maintain such a plugin, consider subscribing to this issue.

Also, does the local documentation count as "external documentation"?

I assume that "external documentation" is documentation outside of the editor.

Maybe, it would be better to create something like experimental/localDocs?

Thank you for your comment.

@Ddystopia Ddystopia force-pushed the open_locally_built_documentatin_instead_of_docs_dot_rs branch from 34be699 to 7831b2b Compare April 26, 2023 18:02
crates/rust-analyzer/src/lsp_ext.rs Outdated Show resolved Hide resolved
crates/ide/src/doc_links.rs Outdated Show resolved Hide resolved
crates/ide/src/doc_links.rs Outdated Show resolved Hide resolved
crates/rust-analyzer/src/handlers.rs Outdated Show resolved Hide resolved
crates/ide/src/doc_links.rs Outdated Show resolved Hide resolved
crates/ide/src/doc_links.rs Outdated Show resolved Hide resolved
crates/ide/src/doc_links.rs Outdated Show resolved Hide resolved
@nemethf
Copy link
Contributor

nemethf commented Apr 27, 2023

As far as I know, rust-analyzer does not provide a stable API interface, so breakage is fine. Also, there is #4604 for this purpose. As stated:

This issue exists to announce changes which might affect rust-analyzer plugins for different editors.
If you maintain such a plugin, consider subscribing to this issue.

Maybe you're right. But to me, this text only says how plugin-maintainers can keep up with the changes of the lsp-extensions and says nothing about a possible breakage.

Also, does the local documentation count as "external documentation"?

I assume that "external documentation" is documentation outside of the editor.

Maybe, it would be better to create something like experimental/localDocs?

I don't know, but I start to wonder why it is necessary to introduce protocol extensions for this feature. The LSP client can send a standard compliant workspace/executeCommand request and the server can send a window/showDocument request right after receiving the executeCommand. The name of the command and its arguments and return value still need to be documented, though.

@Ddystopia Ddystopia requested a review from Veykril April 28, 2023 08:50
@bors
Copy link
Collaborator

bors commented May 1, 2023

☔ The latest upstream changes (presumably #14664) made this pull request unmergeable. Please resolve the merge conflicts.

@Ddystopia Ddystopia force-pushed the open_locally_built_documentatin_instead_of_docs_dot_rs branch from 037798c to 1abf405 Compare May 2, 2023 11:44
@bors
Copy link
Collaborator

bors commented May 2, 2023

☔ The latest upstream changes (presumably #11557) made this pull request unmergeable. Please resolve the merge conflicts.

@Ddystopia Ddystopia force-pushed the open_locally_built_documentatin_instead_of_docs_dot_rs branch from 1abf405 to f24cdf3 Compare May 2, 2023 15:15
crates/ide/src/doc_links.rs Outdated Show resolved Hide resolved
crates/ide/src/doc_links.rs Outdated Show resolved Hide resolved
@Veykril
Copy link
Member

Veykril commented May 2, 2023

two nits, otherwise lgtm, thanks!
@bors delegate+

@bors
Copy link
Collaborator

bors commented May 2, 2023

✌️ @Ddystopia can now approve this pull request

@Veykril
Copy link
Member

Veykril commented May 2, 2023

I don't know, but I start to wonder why it is necessary to introduce protocol extensions for this feature. The LSP client can send a standard compliant workspace/executeCommand request and the server can send a window/showDocument request right after receiving the executeCommand. The name of the command and its arguments and return value still need to be documented, though.

How is using workspace/executeCommand any different? The client still needs to know about the command there that the server supports. And the server replying with window/showDocument would limit in what the client can do, the can't decide to open things in a browser if it wants to for example.

@Ddystopia Ddystopia force-pushed the open_locally_built_documentatin_instead_of_docs_dot_rs branch from f24cdf3 to 2025f17 Compare May 2, 2023 15:27
@Ddystopia
Copy link
Contributor Author

@bors r+

@bors
Copy link
Collaborator

bors commented May 2, 2023

📌 Commit 2025f17 has been approved by Ddystopia

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented May 2, 2023

⌛ Testing commit 2025f17 with merge c9b4116...

@bors
Copy link
Collaborator

bors commented May 2, 2023

☀️ Test successful - checks-actions
Approved by: Ddystopia
Pushing c9b4116 to master...

@bors bors merged commit c9b4116 into rust-lang:master May 2, 2023
10 checks passed
@nemethf
Copy link
Contributor

nemethf commented May 6, 2023

I don't know, but I start to wonder why it is necessary to introduce protocol extensions for this feature. The LSP client can send a standard compliant workspace/executeCommand request and the server can send a window/showDocument request right after receiving the executeCommand. The name of the command and its arguments and return value still need to be documented, though.

How is using workspace/executeCommand any different? The client still needs to know about the command there that the server supports. And the server replying with window/showDocument would limit in what the client can do, the can't decide to open things in a browser if it wants to for example.

As the showDocument request supports an "external" flag, the server can implement the logic of determining how the client should open the documentation. This way the client-side code can be smaller, which is a desirable goal, I think.

At any rate, I'm happy with the merged feature. Thanks @Ddystopia and @Veykril!

bors added a commit that referenced this pull request Oct 10, 2023
…de, r=Veykril

feat: vscode: Support opening local documentation if available

This PR implements the VS code support for opening local documentation (server side support was already implemented in #14662).

[local_docs.webm](https://github.com/rust-lang/rust-analyzer/assets/9659253/715b84dd-4f14-4ba0-a904-749b847eb3d5)

Displaying local instead of web docs can have many benefits:
- the web version may have different features enabled than locally selected
- the standard library may be a different version than is available online
- the user may not be online and therefore cannot access the web documentation
- the documentation may not be available online at all, for example because it is for a new feature in a library the user is currently developing

If the documentation is not available locally, the extension still falls back to the web version.

Closes #12867.

-----

If my implementation isn't really idiomatic TypeScript: Sorry, I'm not much of a TypeScript developer. I am open to feedback, however.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants