vscode: migrate to request type api #3299
Conversation
| * This file mirrors `crates/rust-analyzer/src/req.rs` declarations. | ||
| */ | ||
|
|
||
| import { RequestType, TextDocumentIdentifier, Position, Range, TextDocumentPositionParams, Location, NotificationType, WorkspaceEdit } from "vscode-languageclient"; |
There was a problem hiding this comment.
Followed the same import pattern we have in req.rs
| @@ -0,0 +1,117 @@ | |||
| /** | |||
| * This file mirrors `crates/rust-analyzer/src/req.rs` declarations. | |||
There was a problem hiding this comment.
it's unfortunate that the files are named differently, but I also actively dislike both names :)
There was a problem hiding this comment.
As an aside I really dislike req.rs... it should probably be custom_requests.rs or something with only the imports required to support that file. We end up importing a lot from that file in handlers and conv.
There was a problem hiding this comment.
The name is awesome, unless come up with better!
| tokenSource.token | ||
| ) | ||
| return sendRequestWithRetry(this.ctx.client, ra.inlayHints, request, tokenSource.token) | ||
| .catch(_ => null) |
There was a problem hiding this comment.
Might as well swtich to client.logFailedRequest here and elsewhere?
There was a problem hiding this comment.
Repeating client.logFailedRequest everywhere would be very annoying. What if we add the function that wraps sendRequest and logs on failure or wrap the client with our own RaClient?
class RustAnalyzerClient {
inner: lc.LanguageClient;
async sendRequest(...params) {
return this.inner.sendRequest(...params).catch(err => {
this.inner.logFailedRequest(...);
throw err;
});
}
async sendRequestWithRetry(...params) { ... }
get p2c() { return this.inner.protocol2CodeConverter; }
get c2p() { return this.inner.code2ProtocolConverter; }
/* stuff */
}
| } | ||
|
|
||
| interface InlayHint { | ||
| range: vscode.Range; |
There was a problem hiding this comment.
@SomeoneToIgnore what was the reason to declare vscode.Range instead of lc.Range here? It worked all the time since their shape is structurally the same, but we may not rely on this, so I explicitly added protocol2CodeConverter.asRange() call here...
There was a problem hiding this comment.
No particular reason, pick the one that suits better and works still.
|
|
||
| type Option<T> = null | T; | ||
| type Vec<T> = T[]; | ||
| type FxHashMap<K extends PropertyKey, V> = Record<K, V>; |
There was a problem hiding this comment.
Cheatsheet for Rust -> TS transformations...
| (_error: any) => { | ||
| // FIXME: switch to the more modern (?) typed request infrastructure | ||
| // client.logFailedRequest(OnEnterRequest.type, error); | ||
| return Promise.resolve(null); |
There was a problem hiding this comment.
FYI: @matklad there is no need to wrap the return value into a promise in the then(onfulfilled, onrejected)/catch(onrejected) handlers.
| if (!editor || editor.document.languageId !== 'rust') return; | ||
| if (!ctx.config.highlightingOn) return; | ||
| const client = ctx.client; | ||
| if (!client) return; |
There was a problem hiding this comment.
We should get rid of these if statements, since the client is not nullable today...
But this deserves a separate PR.
There was a problem hiding this comment.
yup, there's FIXME about this in main.ts
|
bors r+ |
Build succeeded |
4 participants
More type-safety to the god of type-safety.