Conversation
- Introduce a command pattern for command handling. - Rewrite installation processes via install/update commands. - Provide flexible method of using buf located in path/storage. - Improve status bar with a command picker. - Support 'buf generate' as a command. - Support module detection for the active file in editor. - Provide a shortcut to opening 'buf.yaml' as a command.
christogav
force-pushed
the
christogav/lsp
branch
from
3d95422 to
9071ecf
Compare
|
Resync'd with #393 - run lint+format and resolve issues. |
doriable
left a comment
doriable
left a comment
There was a problem hiding this comment.
Thank you so much for putting up this PR, we really appreciate this contribution, and apologies for the delayed response/review. I think this is very much in-line with what we are looking for in the LSP integration and in-line with @mcy's first PR.
Our intention is to get these changes checked in. For full transparency, we are currently working on some improvements and changes to our LSP, and do not want to make a release of the VSCode extension backed by the LSP until we are ready, so we may not follow-up with a release immediately. Additionally, this is a pretty big change that would take some time to get merged. @paul-sachs and I will be reviewing this and working with you on this, so don't hesitate to reach out if needed :)
* Remove current buf module concept - update basic statusbar tooltips. * Remove open `buf.yaml` command * Allow user to provide `buf.commandLine.path` and also `buf.commandLine.version` for specific version pinning. * When `buf.commandLine.path` set, do not show `COMMAND_SETUP` commands on the palette. * Move `Log` to its own file. * Remove extra params that we don't want to expose on the extension until we understand more.
1. Change the way findBuf works to align better to PR feedback. 3. If buf not installed, change command on status bar to install buf. 2. Add a `buf.remove` command. 3. Fix fetching github release by version.
|
@doriable @paul-sachs Tidied command descriptions and removed an unnecessary activation event in packages.json - they were hurting my OCD when using the editor. Any issues with the late commit, happy to revert. |
…d in corporate environments.
…x to the public API.
|
Hi team - anymore feedback to address? 🙏 |
doriable
left a comment
doriable
left a comment
There was a problem hiding this comment.
Sorry it took so long for me to get back to this! Left a quick comment, there needs to be a small adjustment to the find/install process, but it's looking good!
src/commands/find-buf.ts
Outdated
| if (version === "latest") { | ||
| found = files | ||
| .filter((f) => f.startsWith("v")) | ||
| .sort() | ||
| .at(-1); |
There was a problem hiding this comment.
This behaviour doesn't seem correct -- if the user has anything installed in their global storage, it would be used as latest, even if it is not actually the latest version of the CLI... so in the case where the user has formerly installed v1.40.0 for example, and then changes the value to latest, no update would actually happen.
There was a problem hiding this comment.
Changed the handling per @paul-sachs's approach slightly modified.
|
Thank you so much for the contribution, I know we put you through a pretty long code review process, so I really appreciate you sticking with us on this! I'm going to merge this PR now 🙏🏼 In the upcoming few weeks, I'm going to figure out some housekeeping around the repository, which may include changing up the Thank you again! |
Fix formatting and missing package-lock file update accidentally merged in with #392
3 participants
Hi team, I've spent the last couple weekends looking at #344 which appears abandoned. This snowballed a bit into a rewrite for mostly maintainability and testability reasons.
Apologies for the size of the pull - happy to split it up if you want to take it on, else I'll continue happily running this locally.
Changes I've introduced:
Support module detection for the active file in editor.Provide a shortcut to opening 'buf.yaml' as a command.