[proposal] design: add LSP interactive refactoring proposals
Hongxiang Jiang (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Alan Donovan
- Madeline Kalil
- Peter Weinberger
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongxiang Jiang (Gerrit)
Hongxiang Jiang uploaded new patchset![]( "Open in Gerrit")
Related details
- Alan Donovan
- Madeline Kalil
- Peter Weinberger
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Donovan (Gerrit)
Alan Donovan added 23 comments![]( "Open in Gerrit")
# Proposal: Language Server Protocol (LSP) Interactive RefactoringIt's more general than this: interactive code actions. They could be for analysis too, not just refactoring.
Worth stating up front, even if the document is primarily framed around refactoring.
Author(s): Hongxiang Jiang (hxj...@golang.org), Alan Donovan (adon...@google.com)You can put me on the second line; you did nearly all the work.
This design puts the language server at the center of the logic—making it the "brain"—while leaving the responsibility of rendering the UI entirely to the language client. To prove the value and viability of this interactive approach, we have successfully built a working prototype based on one of these proposals, which is already released in [gopls v0.22.0](https://github.com/golang/tools/releases/tag/gopls%2Fv0.22.0) and [vscode-go v0.54.0](https://github.com/golang/vscode-go/releases/tag/v0.54.0).This line (with its m-dashes and clunky metaphor) sounds like an LLM wrote it.
This design puts the language server at the center of the logic—making it the "brain"—while leaving the responsibility of rendering the UI entirely to the language client. To prove the value and viability of this interactive approach, we have successfully built a working prototype based on one of these proposals, which is already released in [gopls v0.22.0](https://github.com/golang/tools/releases/tag/gopls%2Fv0.22.0) and [vscode-go v0.54.0](https://github.com/golang/vscode-go/releases/tag/v0.54.0).one of which?
A key component of the design is that the communication mechanism between the language client and the server should be stateless. Consider a metaphor suggested by Alan Donovan: Imagine you go to the DMV to submit an application. You fill in a form full of questions. The officer sees a missing answer and asks you to go back, fill that in, and come back. When you come back to the DMV, you bring the **entire form** instead of only the missing answer.Not my invention, so don't credit me.
A key component of the design is that the communication mechanism between the language client and the server should be stateless. Consider a metaphor suggested by Alan Donovan: Imagine you go to the DMV to submit an application. You fill in a form full of questions. The officer sees a missing answer and asks you to go back, fill that in, and come back. When you come back to the DMV, you bring the **entire form** instead of only the missing answer.Other relevant parts of the DMV metaphor are that
(a) the teller (= server) being stateless permits you to come back later to _any other teller_ (= potentially even a new server process after a restart);
(b) the teller is not tied up while you fill in the form; they can serve another customer; consequently
(c) the teller imposes no time limit (= max RPC duration) on the customer filling in the form.
You should draw the analogy with HTML/HTTP forms, which are traditionally also stateless (though not always in practice).
### Context: How Code Actions WorkBackground:
1. Code action that eventually resolves to a code action with **edits**: The edits are applied directly to the user's workspace by the client.actions ... resolve
ditto in (2)
1. Code action that eventually resolves to a code action with **edits**: The edits are applied directly to the user's workspace by the client.Active voice: "The client applies the edits..."
2. Code action that eventually resolves to a code action with a **command**: For a code action that resolves to a command, that command will be executed, and eventually that command could call `workspace/applyEdit` to ask the client to apply the edit to the workspace.delete
2. Code action that eventually resolves to a code action with a **command**: For a code action that resolves to a command, that command will be executed, and eventually that command could call `workspace/applyEdit` to ask the client to apply the edit to the workspace.perhaps "with a possible side effect of making a server->client `workspace/applyEdit` request"
The proposals below go through these layers from bottom to top to help the reader better understand the thinking logic.approach
### WorkspaceEdit layer (Recommended)Sorry for not raising this question earlier, but let's assume I want to make a non-refactoring feature that is interactive. For example, let's say I want to summon a web-based report about the currently selected code, and the server needs to ask me an additional question. If resolution is done at the edit layer (not code actions or commands), how would this work?
The client re-colllect user input and call `workspaceEdit/resolve` with the new answers.typo
"again collects"
*Note: The original placeholder edits allows [aggressive language clients](https://github.com/microsoft/vscode/issues/106410#issuecomment-690610121) to render a preview of the changes (e.g., when a user hovers over a code action). The actual finalized edit is re-computed by the server by reading `InteractiveParams.data` once the user provides the required input.*eager
"aggressive" means having a tendency to violence. I don't know why it has become a virtuous term in business. (Actually I kinda do.)
*Note: The original placeholder edits allows [aggressive language clients](https://github.com/microsoft/vscode/issues/106410#issuecomment-690610121) to render a preview of the changes (e.g., when a user hovers over a code action). The actual finalized edit is re-computed by the server by reading `InteractiveParams.data` once the user provides the required input.*allow
**Cons:** The main problem with this proposal involves the server-to-client `workspace/applyEdit` request. In LSP, it is generally not a good idea to embed a server-to-client request (up-call) within a client-to-server request (down-call), especially if the up-call may be blocking. When executing a command, the server often holds a non-trivial file system snapshot for static analysis. Holding this snapshot while waiting for user interaction during the back-and-forth communication is inefficient. Additionally, most client-server communications have timeouts, and the request will likely timeout while the user is still deciding on an answer.Up and down here are reversed from how I usually use them (a typical server, on the Internet, is higher than my lowly client computer). Of course, in LSP, the server is often a child process of (hence lower than) the client. I polled pjw for his guess as to the meaning of up and down in LSP, and he refused to play the game on the grounds that the terms are ambiguous. :)
Server-to-client is perfectly clear. Let's use only that style from now on.
**Cons:** The main problem with this proposal involves the server-to-client `workspace/applyEdit` request. In LSP, it is generally not a good idea to embed a server-to-client request (up-call) within a client-to-server request (down-call), especially if the up-call may be blocking. When executing a command, the server often holds a non-trivial file system snapshot for static analysis. Holding this snapshot while waiting for user interaction during the back-and-forth communication is inefficient. Additionally, most client-server communications have timeouts, and the request will likely timeout while the user is still deciding on an answer.It's ok (and quite routine) to do this for nonblocking calls such as applyEdit. The problem comes from making applyEdit a blocking call.
**Cons:** The main problem with this proposal is that it requires embedding a server-to-client request (`window/collectInput`) within an ongoing client-to-server request (such as `workspace/executeCommand` or `codeAction/resolve`). In LSP, it is generally not a good idea to embed a blocking up-call within a down-call. When executing a command or resolving a code action, the server often holds a non-trivial file system snapshot for static analysis. Holding this snapshot while waiting for the user to provide input is inefficient. Furthermore, most client-server communications have timeouts, and the request will likely timeout while the user is still deciding on an answer.time out (noun = 1 word, verb = 2)
Related details
- Hongxiang Jiang
- Madeline Kalil
- Peter Weinberger
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongxiang Jiang (Gerrit)
Hongxiang Jiang uploaded new patchset![]( "Open in Gerrit")
Related details
- Hongxiang Jiang
- Madeline Kalil
- Peter Weinberger
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
SLSA Policy Verification Service (Gerrit)
SLSA Policy Verification Service voted SLSA-Policy-Verified+1![]( "Open in Gerrit")
| SLSA-Policy-Verified | +1 |
Related details
- Hongxiang Jiang
- Madeline Kalil
- Peter Weinberger
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongxiang Jiang (Gerrit)
Hongxiang Jiang uploaded new patchset![]( "Open in Gerrit")
Related details
- Hongxiang Jiang
- Madeline Kalil
- Peter Weinberger
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongxiang Jiang (Gerrit)
Hongxiang Jiang uploaded new patchset![]( "Open in Gerrit")
Related details
- Hongxiang Jiang
- Madeline Kalil
- Peter Weinberger
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongxiang Jiang (Gerrit)
Hongxiang Jiang uploaded new patchset![]( "Open in Gerrit")
Related details
- Hongxiang Jiang
- Madeline Kalil
- Peter Weinberger
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongxiang Jiang (Gerrit)
Hongxiang Jiang uploaded new patchset![]( "Open in Gerrit")
Related details
- Hongxiang Jiang
- Madeline Kalil
- Peter Weinberger
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongxiang Jiang (Gerrit)
Hongxiang Jiang uploaded new patchset![]( "Open in Gerrit")
Related details
- Hongxiang Jiang
- Madeline Kalil
- Peter Weinberger
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongxiang Jiang (Gerrit)
Hongxiang Jiang voted and added 22 comments![]( "Open in Gerrit")
Votes added by Hongxiang Jiang
| Commit-Queue | +1 |
22 comments
# Proposal: Language Server Protocol (LSP) Interactive RefactoringIt's more general than this: interactive code actions. They could be for analysis too, not just refactoring.
Worth stating up front, even if the document is primarily framed around refactoring.
I don't fully understand the required doc change to resolve this comment. Could you take a look and let me know is the change is intended.
Our thinking logic start by focusing on "code actions" and then we kind of expand a little bit to other LSP operations. Could you elaborate (possibly with a quick example) more about what "analysis" UX will looks like?
Author(s): Hongxiang Jiang (hxj...@golang.org), Alan Donovan (adon...@google.com)You can put me on the second line; you did nearly all the work.
Acknowledged
This design puts the language server at the center of the logic—making it the "brain"—while leaving the responsibility of rendering the UI entirely to the language client. To prove the value and viability of this interactive approach, we have successfully built a working prototype based on one of these proposals, which is already released in [gopls v0.22.0](https://github.com/golang/tools/releases/tag/gopls%2Fv0.22.0) and [vscode-go v0.54.0](https://github.com/golang/vscode-go/releases/tag/v0.54.0).This line (with its m-dashes and clunky metaphor) sounds like an LLM wrote it.
And the fact is, it is LLM wrote. :D
I write the first draft and it give me a feeling of "unprofessional" so I decided to ask LLM to refactor this for me.
PTAL.
This design puts the language server at the center of the logic—making it the "brain"—while leaving the responsibility of rendering the UI entirely to the language client. To prove the value and viability of this interactive approach, we have successfully built a working prototype based on one of these proposals, which is already released in [gopls v0.22.0](https://github.com/golang/tools/releases/tag/gopls%2Fv0.22.0) and [vscode-go v0.54.0](https://github.com/golang/vscode-go/releases/tag/v0.54.0).Hongxiang Jiangone of which?
Done
A key component of the design is that the communication mechanism between the language client and the server should be stateless. Consider a metaphor suggested by Alan Donovan: Imagine you go to the DMV to submit an application. You fill in a form full of questions. The officer sees a missing answer and asks you to go back, fill that in, and come back. When you come back to the DMV, you bring the **entire form** instead of only the missing answer.Other relevant parts of the DMV metaphor are that
(a) the teller (= server) being stateless permits you to come back later to _any other teller_ (= potentially even a new server process after a restart);
(b) the teller is not tied up while you fill in the form; they can serve another customer; consequently
(c) the teller imposes no time limit (= max RPC duration) on the customer filling in the form.
Done. I added this "benefit" section after the "result" section.
However, I think (a) is not really applicable in the context of LSP. Once the server disconnect, the client will have to re-connect, and I assume the client will also automatically cancel any pending requests.
A key component of the design is that the communication mechanism between the language client and the server should be stateless. Consider a metaphor suggested by Alan Donovan: Imagine you go to the DMV to submit an application. You fill in a form full of questions. The officer sees a missing answer and asks you to go back, fill that in, and come back. When you come back to the DMV, you bring the **entire form** instead of only the missing answer.Not my invention, so don't credit me.
Done
You should draw the analogy with HTML/HTTP forms, which are traditionally also stateless (though not always in practice).
I just mentioned "modeled after stateless HTML/HTTP forms". I'm not sure if there is anything else you want to mention.
### Context: How Code Actions WorkHongxiang JiangBackground:
Done
1. Code action that eventually resolves to a code action with **edits**: The edits are applied directly to the user's workspace by the client.Active voice: "The client applies the edits..."
Done
1. Code action that eventually resolves to a code action with **edits**: The edits are applied directly to the user's workspace by the client.actions ... resolve
ditto in (2)
Done
2. Code action that eventually resolves to a code action with a **command**: For a code action that resolves to a command, that command will be executed, and eventually that command could call `workspace/applyEdit` to ask the client to apply the edit to the workspace.perhaps "with a possible side effect of making a server->client `workspace/applyEdit` request"
Done
2. Code action that eventually resolves to a code action with a **command**: For a code action that resolves to a command, that command will be executed, and eventually that command could call `workspace/applyEdit` to ask the client to apply the edit to the workspace.Hongxiang Jiangdelete
Done
The proposals below go through these layers from bottom to top to help the reader better understand the thinking logic.Hongxiang Jiangapproach
Done
Sorry for not raising this question earlier, but let's assume I want to make a non-refactoring feature that is interactive. For example, let's say I want to summon a web-based report about the currently selected code, and the server needs to ask me an additional question. If resolution is done at the edit layer (not code actions or commands), how would this work?
Short answer is, it will not work...
The solution only works if there is an "edit" because the interactive layer is right before the edit got applied to the workspace. The input and output of interactive refactoring are both "edit".
However, the command layer, on the other hand, the input and output is "command" and potentially expanding to other operations like "rename"... That will be able to cover your use case.
To compare command layer and edit layer, I think only con is the command layer does not cover code action resolve to edits. We can discuss this offline further on this topic.
The client re-colllect user input and call `workspaceEdit/resolve` with the new answers.Hongxiang Jiangtypo
"again collects"
Done
*Note: The original placeholder edits allows [aggressive language clients](https://github.com/microsoft/vscode/issues/106410#issuecomment-690610121) to render a preview of the changes (e.g., when a user hovers over a code action). The actual finalized edit is re-computed by the server by reading `InteractiveParams.data` once the user provides the required input.*eager
"aggressive" means having a tendency to violence. I don't know why it has become a virtuous term in business. (Actually I kinda do.)
Done
*Note: The original placeholder edits allows [aggressive language clients](https://github.com/microsoft/vscode/issues/106410#issuecomment-690610121) to render a preview of the changes (e.g., when a user hovers over a code action). The actual finalized edit is re-computed by the server by reading `InteractiveParams.data` once the user provides the required input.*Hongxiang Jiangallow
Done
**Cons:** The main problem with this proposal involves the server-to-client `workspace/applyEdit` request. In LSP, it is generally not a good idea to embed a server-to-client request (up-call) within a client-to-server request (down-call), especially if the up-call may be blocking. When executing a command, the server often holds a non-trivial file system snapshot for static analysis. Holding this snapshot while waiting for user interaction during the back-and-forth communication is inefficient. Additionally, most client-server communications have timeouts, and the request will likely timeout while the user is still deciding on an answer.It's ok (and quite routine) to do this for nonblocking calls such as applyEdit. The problem comes from making applyEdit a blocking call.
Done
**Cons:** The main problem with this proposal involves the server-to-client `workspace/applyEdit` request. In LSP, it is generally not a good idea to embed a server-to-client request (up-call) within a client-to-server request (down-call), especially if the up-call may be blocking. When executing a command, the server often holds a non-trivial file system snapshot for static analysis. Holding this snapshot while waiting for user interaction during the back-and-forth communication is inefficient. Additionally, most client-server communications have timeouts, and the request will likely timeout while the user is still deciding on an answer.Up and down here are reversed from how I usually use them (a typical server, on the Internet, is higher than my lowly client computer). Of course, in LSP, the server is often a child process of (hence lower than) the client. I polled pjw for his guess as to the meaning of up and down in LSP, and he refused to play the game on the grounds that the terms are ambiguous. :)
Server-to-client is perfectly clear. Let's use only that style from now on.
When I first join the team, I find soooo confusing regarding "up" and "down" because my mental model tells me server is up in the cloud...
Later after working in gopls for 1 year, I start to get used to the mental model like yours. The server is running below the IDE (client).
I have removed the "up" and "down" to avoid any confusion.
**Cons:** The main problem with this proposal is that it requires embedding a server-to-client request (`window/collectInput`) within an ongoing client-to-server request (such as `workspace/executeCommand` or `codeAction/resolve`). In LSP, it is generally not a good idea to embed a blocking up-call within a down-call. When executing a command or resolving a code action, the server often holds a non-trivial file system snapshot for static analysis. Holding this snapshot while waiting for the user to provide input is inefficient. Furthermore, most client-server communications have timeouts, and the request will likely timeout while the user is still deciding on an answer.time out (noun = 1 word, verb = 2)
- Alan Donovan
- Madeline Kalil
- Peter Weinberger
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongxiang Jiang (Gerrit)
Hongxiang Jiang uploaded new patchset![]( "Open in Gerrit")
- Alan Donovan
- Hongxiang Jiang
- Madeline Kalil
- Peter Weinberger
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongxiang Jiang (Gerrit)
Hongxiang Jiang uploaded new patchset![]( "Open in Gerrit")
- Alan Donovan
- Madeline Kalil
- Peter Weinberger
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongxiang Jiang (Gerrit)
Hongxiang Jiang uploaded new patchset![]( "Open in Gerrit")
Related details
- Alan Donovan
- Madeline Kalil
- Peter Weinberger
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongxiang Jiang (Gerrit)
Hongxiang Jiang uploaded new patchset![]( "Open in Gerrit")
Related details
- Alan Donovan
- Madeline Kalil
- Peter Weinberger
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danny Tuppeny (Gerrit)
Danny Tuppeny added 2 comments![]( "Open in Gerrit")
Thank you for working on this! The changes all look good to me, with one small question.
filters?: string[];In the VS Code APIs, the filters have labels and sets of extensions like:
```js
{
'Images': ['png', 'jpg'],
'TypeScript': ['ts', 'tsx']
}
```
Should we do the same?
Related details
- Alan Donovan
- Hongxiang Jiang
- Madeline Kalil
- Peter Weinberger
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil added 2 comments![]( "Open in Gerrit")
optional?: boolean;Did you consider calling this "required" instead? It would make it a little bit easier to understand the default value in my opinion. Or is it preferred to have the default be that the form field is required if you don't specify it (and not having to always specify "required": true)?
{id: "tags", descript: "tags to add", kind: string},description?
Related details
- Alan Donovan
- Hongxiang Jiang
- Peter Weinberger
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongxiang Jiang (Gerrit)
Hongxiang Jiang uploaded new patchset![]( "Open in Gerrit")
Related details
- Alan Donovan
- Hongxiang Jiang
- Peter Weinberger
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongxiang Jiang (Gerrit)
Hongxiang Jiang added 4 comments![]( "Open in Gerrit")
Update the imagines after the patch. TODO(hxjiang).
Did you consider calling this "required" instead? It would make it a little bit easier to understand the default value in my opinion. Or is it preferred to have the default be that the form field is required if you don't specify it (and not having to always specify "required": true)?
I'm a little bit struggling to decide which way is better. I would imagine most of the questions are required.
But finally, I think required might be better just to avoid double negative.
In the VS Code APIs, the filters have labels and sets of extensions like:
```js
{
'Images': ['png', 'jpg'],
'TypeScript': ['ts', 'tsx']
}
```Should we do the same?
Hi Danny, I read the vscode api and decide not to use the same struct layer simply because we are defining the protocol not UI. I can see the value of having the grouping filters (images for png and jpg)... But the goal of the filter is to tell the user, these are the files that can be selected. I can imagine for other LSP clients, the label `Imagines` or `Typescript` is not going to be useful as they may choose to use a search based solution.
I personally want to keep the protocol as "simple" as possible and avoid any UI elements in the protocol definition.
{id: "tags", descript: "tags to add", kind: string},Hongxiang Jiangdescription?
Done
Related details
- Alan Donovan
- Danny Tuppeny
- Madeline Kalil
- Peter Weinberger
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danny Tuppeny (Gerrit)
Danny Tuppeny added 2 comments![]( "Open in Gerrit")
optional?: boolean;Hongxiang JiangDid you consider calling this "required" instead? It would make it a little bit easier to understand the default value in my opinion. Or is it preferred to have the default be that the form field is required if you don't specify it (and not having to always specify "required": true)?
I'm a little bit struggling to decide which way is better. I would imagine most of the questions are required.
But finally, I think required might be better just to avoid double negative.
I don't feel strongly, but I do think `optional` is better if questions are required if the field is omitted. Otherwise, the field is required "if true or omitted" which I think is confusing.
Another option is to make the field required to avoid any ambiguity (I don't think payload size is a concern here, the number of fields is low), although if you've already shipped something for Go you'd need to handle it missing for compatibility even if not in-spec.
filters?: string[];Hongxiang JiangIn the VS Code APIs, the filters have labels and sets of extensions like:
```js
{
'Images': ['png', 'jpg'],
'TypeScript': ['ts', 'tsx']
}
```Should we do the same?
Hi Danny, I read the vscode api and decide not to use the same struct layer simply because we are defining the protocol not UI. I can see the value of having the grouping filters (images for png and jpg)... But the goal of the filter is to tell the user, these are the files that can be selected. I can imagine for other LSP clients, the label `Imagines` or `Typescript` is not going to be useful as they may choose to use a search based solution.
I personally want to keep the protocol as "simple" as possible and avoid any UI elements in the protocol definition.
That's fine, I just wanted to check it wasn't overlooked.
It does mean we need to decide what to pass to the VS Code API though - I believe it shows as "Images (*.bmp, *.jpg)" so we'd need to decide what to put in place of "Images" (we could use the extensions, but then they'd be duplicated).
Related details
- Alan Donovan
- Hongxiang Jiang
- Madeline Kalil
- Peter Weinberger
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongxiang Jiang (Gerrit)
Hongxiang Jiang uploaded new patchset![]( "Open in Gerrit")
Related details
- Alan Donovan
- Hongxiang Jiang
- Madeline Kalil
- Peter Weinberger
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongxiang Jiang (Gerrit)
Hongxiang Jiang added 1 comment![]( "Open in Gerrit")
optional?: boolean;Hongxiang JiangDid you consider calling this "required" instead? It would make it a little bit easier to understand the default value in my opinion. Or is it preferred to have the default be that the form field is required if you don't specify it (and not having to always specify "required": true)?
Danny TuppenyI'm a little bit struggling to decide which way is better. I would imagine most of the questions are required.
But finally, I think required might be better just to avoid double negative.
I don't feel strongly, but I do think `optional` is better if questions are required if the field is omitted. Otherwise, the field is required "if true or omitted" which I think is confusing.
Another option is to make the field required to avoid any ambiguity (I don't think payload size is a concern here, the number of fields is low), although if you've already shipped something for Go you'd need to handle it missing for compatibility even if not in-spec.
- Alan Donovan
- Danny Tuppeny
- Madeline Kalil
- Peter Weinberger
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongxiang Jiang (Gerrit)
Hongxiang Jiang uploaded new patchset![]( "Open in Gerrit")
Related details
- Alan Donovan
- Danny Tuppeny
- Madeline Kalil
- Peter Weinberger
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danny Tuppeny (Gerrit)
Danny Tuppeny added 1 comment![]( "Open in Gerrit")
// The client must include answers for all required fields (where `required`
// is true). Answers for optional fields (where `required` is false)Is this bit necessary? Since the server can re-present fields that are incomplete and validation can only occur on a round trip to the server, would it be better to allow a client to re-resolve after each question is answered, so validation can be done for each question instead of just once at the end?
Related details
- Alan Donovan
- Hongxiang Jiang
- Madeline Kalil
- Peter Weinberger
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongxiang Jiang (Gerrit)
Hongxiang Jiang added 1 comment![]( "Open in Gerrit")
// The client must include answers for all required fields (where `required`
// is true). Answers for optional fields (where `required` is false)Is this bit necessary? Since the server can re-present fields that are incomplete and validation can only occur on a round trip to the server, would it be better to allow a client to re-resolve after each question is answered, so validation can be done for each question instead of just once at the end?
I think it should be ok. I have changed the description accordingly.
Basically, this will be a "keen" applicant who keeps ask officer to valid their form and resubmit it after each answer he fills in.
But I have one case, imagine the server have 5 questions, the last question is optional.
The clinet take the strategy to ask one question at a time and resolve after each time the user fill in the answer. After 4 answers collected, the client call resolve, the server notice all the "required" questions are answered. And the optional question is not answered. So the server think all required is collected. but the client never show the user the optional question.
Related details
- Alan Donovan
- Danny Tuppeny
- Madeline Kalil
- Peter Weinberger
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongxiang Jiang (Gerrit)
Hongxiang Jiang added 1 comment![]( "Open in Gerrit")
// The client must include answers for all required fields (where `required`
// is true). Answers for optional fields (where `required` is false)Hongxiang JiangIs this bit necessary? Since the server can re-present fields that are incomplete and validation can only occur on a round trip to the server, would it be better to allow a client to re-resolve after each question is answered, so validation can be done for each question instead of just once at the end?
I think it should be ok. I have changed the description accordingly.
Basically, this will be a "keen" applicant who keeps ask officer to valid their form and resubmit it after each answer he fills in.
But I have one case, imagine the server have 5 questions, the last question is optional.
The clinet take the strategy to ask one question at a time and resolve after each time the user fill in the answer. After 4 answers collected, the client call resolve, the server notice all the "required" questions are answered. And the optional question is not answered. So the server think all required is collected. but the client never show the user the optional question.
On the other hand, the server may execute the flow below to achieve step by step validation.
```
Server -> Client: Q1
Client -> Server: A1
Server -> Client: Q1, Q2, A1
Client -> Server: Q1, Q2, A1, A2
```
Hongxiang Jiang (Gerrit)
Hongxiang Jiang added 1 comment![]( "Open in Gerrit")
// The client must include answers for all required fields (where `required`
// is true). Answers for optional fields (where `required` is false)Hongxiang JiangIs this bit necessary? Since the server can re-present fields that are incomplete and validation can only occur on a round trip to the server, would it be better to allow a client to re-resolve after each question is answered, so validation can be done for each question instead of just once at the end?
Hongxiang JiangI think it should be ok. I have changed the description accordingly.
Basically, this will be a "keen" applicant who keeps ask officer to valid their form and resubmit it after each answer he fills in.
But I have one case, imagine the server have 5 questions, the last question is optional.
The clinet take the strategy to ask one question at a time and resolve after each time the user fill in the answer. After 4 answers collected, the client call resolve, the server notice all the "required" questions are answered. And the optional question is not answered. So the server think all required is collected. but the client never show the user the optional question.
On the other hand, the server may execute the flow below to achieve step by step validation.
```
Server -> Client: Q1
Client -> Server: A1
Server -> Client: Q1, Q2, A1
Client -> Server: Q1, Q2, A1, A2
```
So the server can trust the client have shown all the questions to the user, the server controls how the questions get rolled out to the user. The client is simply follow order to show the questions and collect the answers.
Danny Tuppeny (Gerrit)
Danny Tuppeny added 1 comment![]( "Open in Gerrit")
// The client must include answers for all required fields (where `required`
// is true). Answers for optional fields (where `required` is false)Hongxiang JiangIs this bit necessary? Since the server can re-present fields that are incomplete and validation can only occur on a round trip to the server, would it be better to allow a client to re-resolve after each question is answered, so validation can be done for each question instead of just once at the end?
Hongxiang JiangI think it should be ok. I have changed the description accordingly.
Basically, this will be a "keen" applicant who keeps ask officer to valid their form and resubmit it after each answer he fills in.
But I have one case, imagine the server have 5 questions, the last question is optional.
The clinet take the strategy to ask one question at a time and resolve after each time the user fill in the answer. After 4 answers collected, the client call resolve, the server notice all the "required" questions are answered. And the optional question is not answered. So the server think all required is collected. but the client never show the user the optional question.
Hongxiang JiangOn the other hand, the server may execute the flow below to achieve step by step validation.
```
Server -> Client: Q1
Client -> Server: A1
Server -> Client: Q1, Q2, A1
Client -> Server: Q1, Q2, A1, A2
```
So the server can trust the client have shown all the questions to the user, the server controls how the questions get rolled out to the user. The client is simply follow order to show the questions and collect the answers.
After 4 answers collected, the client call resolve, the server notice all the "required" questions are answered. And the optional question is not answered. So the server think all required is collected. but the client never show the user the optional question.
That's a good point. One possible fix is to require the client to add an entry to `formAnswers` with no `value` to indicate the user had been presented with the question and chose not to answer it, but I don't know if it's worth it.
> On the other hand, the server may execute the flow below to achieve step by step validation.
That works for sequential clients, but I think wouldn't woek well for "whole dialog" clients, because they wouldn't get to show everything at once?
I don't have a strong preference either way here, I think our use will be simple enough that the difference probably isn't large, so I'm happy to go with what you think is best (I just thought it was worth raising in case it hadn't been considered).
Thanks!
Related details
- Alan Donovan
- Hongxiang Jiang
- Madeline Kalil
- Peter Weinberger
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongxiang Jiang (Gerrit)
Hongxiang Jiang added 1 comment![]( "Open in Gerrit")
// The client must include answers for all required fields (where `required`
// is true). Answers for optional fields (where `required` is false)Hongxiang JiangIs this bit necessary? Since the server can re-present fields that are incomplete and validation can only occur on a round trip to the server, would it be better to allow a client to re-resolve after each question is answered, so validation can be done for each question instead of just once at the end?
Hongxiang JiangI think it should be ok. I have changed the description accordingly.
Basically, this will be a "keen" applicant who keeps ask officer to valid their form and resubmit it after each answer he fills in.
But I have one case, imagine the server have 5 questions, the last question is optional.
The clinet take the strategy to ask one question at a time and resolve after each time the user fill in the answer. After 4 answers collected, the client call resolve, the server notice all the "required" questions are answered. And the optional question is not answered. So the server think all required is collected. but the client never show the user the optional question.
Hongxiang JiangOn the other hand, the server may execute the flow below to achieve step by step validation.
```
Server -> Client: Q1
Client -> Server: A1
Server -> Client: Q1, Q2, A1
Client -> Server: Q1, Q2, A1, A2
```
Danny TuppenySo the server can trust the client have shown all the questions to the user, the server controls how the questions get rolled out to the user. The client is simply follow order to show the questions and collect the answers.
After 4 answers collected, the client call resolve, the server notice all the "required" questions are answered. And the optional question is not answered. So the server think all required is collected. but the client never show the user the optional question.
That's a good point. One possible fix is to require the client to add an entry to `formAnswers` with no `value` to indicate the user had been presented with the question and chose not to answer it, but I don't know if it's worth it.
> On the other hand, the server may execute the flow below to achieve step by step validation.That works for sequential clients, but I think wouldn't woek well for "whole dialog" clients, because they wouldn't get to show everything at once?
I don't have a strong preference either way here, I think our use will be simple enough that the difference probably isn't large, so I'm happy to go with what you think is best (I just thought it was worth raising in case it hadn't been considered).Thanks!
One possible fix is to require the client to add an entry to formAnswers with no value to indicate the user had been presented with the question and chose not to answer it, but I don't know if it's worth it.
I feel this is a little bit over-engineered.
That works for sequential clients, but I think wouldn't woek well for "whole dialog" clients, because they wouldn't get to show everything at once?
Yes, this is also my concern. We can't tell the language client what to do to collect the answer. But we can put a clear recommendation to ask language client to show the missing answer question or the invalid answer question.
In addition, I think even for vscode, we can turn an "enum" selection to an answer selection. Instead of automatically showing only the question missing answer, we can show a enum selection with all the questions
```
- Q1: what is the tags? Invalid.
- Q2: what is the transformation? Valid.
- Submit
- ```
and let the user select which question they want to re-answer. So as long as a client support "enum", the client should be able to show the "whole dialog".
The idea just came across my mind and I have not yet spent enough time to implement it. But I imagine for simple questions with only one or two questions, displaying the entire dialog is not the perfect UX for the user.
Related details
- Alan Donovan
- Danny Tuppeny
- Madeline Kalil
- Peter Weinberger
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danny Tuppeny (Gerrit)
Danny Tuppeny added 1 comment![]( "Open in Gerrit")
In addition, I think even for vscode, we can turn an "enum" selection to an answer selection. Instead of automatically showing only the question missing answer, we can show a enum selection with all the questions
That's an interesting idea, and actually similar to the "settings editor" we have for the "Flutter: New Project" settings:
https://dartcode.org/releases/v3-26/#settings-for-flutter-new-project
Although I think doing this from the server would result in a sub-optimal experience for clients that _could_ show a whole dialog. I wonder whether we should add a flag to capabilities for a client to indicate whether it's a single-input or multi-input client so the server can provide the best experience for each?
Related details
- Alan Donovan
- Hongxiang Jiang
- Madeline Kalil
- Peter Weinberger
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |