Remove configs from extension #493
Conversation
README.md
Outdated
| | Use `buf` at specified path. | User specified path | {empty} | | ||
| | Install and use the specified version of `buf`. | {empty} | User specified semver version | | ||
| | Use `buf` at specified path and display an error message. | User specified path | User specified semver version | | ||
| `$PATH`. However, you don't have to install `buf` if you don't have it. If the extension does |
There was a problem hiding this comment.
Simplified version of this entire paragraph that says the important part first:
You do not need to install the Buf CLI to use this extension. By default, the extension uses the Buf CLI from your system $PATH. If buf isn't found on your $PATH, the extension automatically downloads and installs the latest version to its own storage directory.
package.json
Outdated
| "buf.commandLine.path": { | ||
| "type": "string", | ||
| "description": "The path to a specific install of Buf to use. Relative paths are supported and are relative to the workspace root." | ||
| "description": "The path to a specific install of Buf to use. Relative paths are supported and are relative to the workspace root.", |
There was a problem hiding this comment.
Inconsistency: description says "Buf" but deprecation message refers to it as "buf CLI".
There was a problem hiding this comment.
Fixed this to just use "Buf" to refer to the the CLI binary unless there are other circumstances. Made the same change for the version config below.
package.json
Outdated
| "type": "boolean", | ||
| "default": true, | ||
| "description": "Automatically restart Buf (up to 4 times) if it crashes." | ||
| "description": "Automatically restart Buf (up to 4 times) if it crashes.", |
There was a problem hiding this comment.
Confusion: description says it restarts "Buf" but deprecation message says it restarts the "Buf language server." Which restarts?
There was a problem hiding this comment.
Changed the description to include "language server" since that is the correct thing.
package.json
Outdated
| "type": "boolean", | ||
| "default": false, | ||
| "description": "Enable debug mode." | ||
| "description": "Enable debug logs for Buf language server." |
There was a problem hiding this comment.
Nit: a prior message referred to "the" Buf language server but this says "for Buf language server."
There was a problem hiding this comment.
This should have a "the", added.
We are already testing the code path for installation in the integration tests, where we are able to set up a mock server to grab a buf CLI binary installed to the workspace via `npm install`. In the case of playwright tests, everything runs in a sandbox, and we do not have access to the buf binary set up outside of the test. We'll temporarily disable these tests until we can find a good solution for this.
| url: `${assetDownloadURL}buf-win32-arm64`, | ||
| }, | ||
| ], | ||
| }); |
There was a problem hiding this comment.
The downside to this is that because the tests run in a context that cannot import vscode package, we can no longer assert against Release in src/github.ts. But I think it's generally okay.
package.json
Outdated
| "description": "Specific version (git tag e.g. 'v1.53.0') of Buf release to download and install.", | ||
| "deprecationMessage": "Deprecated: Buf version is no longer configurable." | ||
| }, | ||
| "buf.restartAfterCrash": { |
There was a problem hiding this comment.
Almost all of these options were supposed to be completely deleted.
There was a problem hiding this comment.
We want to make sure that the extension continues to be compatible with any existing settings.json files that users may have checked into their projects and/or configured locally. Deprecating them removes them from the settings page of the extension and shows the deprecation notice when editing the settings.json file in VS Code.
There was a problem hiding this comment.
These options should have never existed, and we should not have to support them or account for them in the future. Delete them entirely.
Not a huge fan of this strategy, since it could increase the overall time to run CI/tests, but also, I think might help make the tests that rely on "downloading" the binary through the msw server a little bit more stable (give it a little bit more time for I/O to grab the binary into the extension cache).
jrinehart-buf
left a comment
jrinehart-buf
left a comment
There was a problem hiding this comment.
Spotted two nits, both fixable w/ suggestions.
Co-authored-by: Joe Rinehart <[email protected]>
A decison was made to deprecate all the configurations
from the extension other than turning debug logs on and
off. The new behaviour for managing the
bufCLI binaryis that the extension checks the user's system
$PATHfor
buf, and uses that. Otherwise, the extension willdownload the latest
bufCLI version to the extensionglobal storage.
This PR updates the GitHub Actions workflow to also include
a branch that does not install the
bufCLI to the$PATHtoexercise the download code path. It also updates the
READMEto reflect the settings changes.