Auto-save codeAction changes to __package.rb files #7214
Conversation
We have some options here. Another option would be to implement something like "save all files that weren't open before we applied the WorkspaceEdit," but that's a little tricky, because we'd have to compute the list of open files on the server side--by the time the WorkspaceEdit has applied, VS Code will already have the files open, so we can't do it purely in the client. In any case, this isn't a trapdoor decision--we can always decide to change our minds and expand this feature if people start to ask for it.
|
We have a policy of testing changes to Sorbet against Stripe's codebase before Stripe employees can see the build results here: → https://go/builds/bui_OPtucaQdie9lH1 |
|
One concern with this new command, is that "Save all __package.rb" files seems like a maybe too-specific general use command. If the only goal is to call it internally here Then maybe defining as an internal command would be better - just don't define it in the package.json, the commandId is available for API calls still and activation should not be an issue since the extension would have to already be active for the command to be needed (in theory you can prefix the Id with |
This prevents it from being shown in the command picker, but still lets it be called from the extension. This reduces the public surface area of the extension. We can always make this public later if it's something we want. But starting with it private makes it easier for us to change our minds and either expand or limit the scope of how this command works.
73440e0 to
cf06a06
Compare
| @@ -7,6 +7,7 @@ import { setLogLevel } from "./commands/setLogLevel"; | |||
| import { showSorbetActions } from "./commands/showSorbetActions"; | |||
| import { showSorbetConfigurationPicker } from "./commands/showSorbetConfigurationPicker"; | |||
| import { toggleUntypedCodeHighlighting } from "./commands/toggleUntypedCodeHighlighting"; | |||
| import { savePackageFiles } from "./commands/savePackageFiles"; | |||
There was a problem hiding this comment.
Nit: project file imports are sorted by import path, can this be moved to line 6?
* no-op: Operate on FileRef more * no-op: Mark some generated code as const * Server changes We have some options here. Another option would be to implement something like "save all files that weren't open before we applied the WorkspaceEdit," but that's a little tricky, because we'd have to compute the list of open files on the server side--by the time the WorkspaceEdit has applied, VS Code will already have the files open, so we can't do it purely in the client. In any case, this isn't a trapdoor decision--we can always decide to change our minds and expand this feature if people start to ask for it. * Client changes * Don't register as a command in package.json This prevents it from being shown in the command picker, but still lets it be called from the extension. This reduces the public surface area of the extension. We can always make this public later if it's something we want. But starting with it private makes it easier for us to change our minds and either expand or limit the scope of how this command works. * Move log to trace level * Another warning -> trace log * sort
Implementation
We have some options here. Another option would be to implement something like
"save all files that weren't open before we applied the WorkspaceEdit," but
that's a little tricky, because we'd have to compute the list of open files on
the server side--by the time the WorkspaceEdit has applied, VS Code will already
have the files open, so we can't do it purely in the client.
In any case, this isn't a trapdoor decision--we can always decide to change our
minds and expand this feature if people start to ask for it.
Motivation
Requested by a user
Test plan
I only tested this manually.
Screen.Recording.2023-08-08.at.4.50.53.PM.mov
It's not particularly easy to test these changes in a meaningful way on the
server, because we don't have a concept of "edited but not saved files" in our
test harness.
It's possible that this is something we could write a VS Code test for? I'm not
super familiar with how one might work. @damolina-stripe, do you have thoughts
on whether/how to test the client-side portions of this change?
Client changes in non-VS Code editors
I found this comment from a maintainer of the LSP spec to be useful. The idea seems to be that clients should control maintaining a registry of commands, mapping to their implementations. Servers can kick-start this registry using the
executeCommandProvidercapability on initialization if it's desired to offload the implementation of a command to the server.In our case, the thing we want relies entirely on the client, so we can't have this command implemented on the server.
I did confirm that at least in Neovim's built-in language client, they support declaring a string -> callback function registry, which is basically the same as the API VS Code gives us with
registerCommand. I'm guessing that means that other language clients will be able to implement this as well.