Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VSCode: avoid errors upon completion requests #1627

Merged
merged 3 commits into from
Jan 3, 2024

Conversation

barna-emarsys
Copy link
Contributor

@barna-emarsys barna-emarsys commented Dec 28, 2023

While authoring a .sqlx file, the VS Code Dataform extension keeps showing error messages about failed completion requests. They read:

  Message: Unhandled method textDocument/completion
  Code: -32601

Here's a demonstration:
dataform_compile_crash_demostration

As far as I see, the issue is caused by the Dataform LSP declaring itself as a completion provider, but never providing a method that provides completions. I could not find any part of the source code that handles completions (the implemented hooks are onInitialized, onRequest("compile"), onDidSave and onDefinition). It also seems to me the declarative parts of a language extension can not be responsible for providing completions.

The extension lists as its features: syntax highlighting, compilation and go to definition. All those features continue to work without the LSP declaring itself as a completion provider.

This PR resolves this issue by removing the completion provider declaration.

PS: I have two more PR's ready with suggested features, but I didn't want to overload you with 3 separate PR's at once.
They would:

  • add a configuration setting for specifying the location of the dataform folder within the project
  • add a configuration setting to disable compile on save

Are you open to reviewing those as well?

Copy link

google-cla bot commented Dec 28, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@barna-emarsys
Copy link
Contributor Author

@GJMcGowan, since you have worked most on the VS Code extension, maybe you can review this PR?

Copy link
Collaborator

@GJMcGowan GJMcGowan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR!

You probably noticed that we haven't published a new version of this extension in a while, as we aren't prioritising it's development. That said, this is a good fix and I'm happy to review future work.

I've now submitted a fix that allows the extension to actually build, so I would encourage you in future PRs to build the extension locally and try out the changes (use the commands in CONTRIBUTING.md to create a .vsix file and then install it)

@barna-emarsys
Copy link
Contributor Author

barna-emarsys commented Jan 2, 2024

Thanks for the quick review!

I was actually able to build the .vsix file without modifications to the build. I did, however, need to use Docker to virtualize an x64 Linux environment since I'm on an arm mac (see: #1593). Since this is my first time with the Bazel build system, I didn't attempt to make it work on Apple silicon.

These are the steps I took:

  1. I added the following docker-compose file to the root repository (docker-compose.yaml):
version: "3.0"

services:
  dev:
    image: node:16
    tty: true
    working_dir: /usr/src/app
    platform: linux/amd64
    volumes:
      - .:/usr/src/app
docker compose up -d
docker compose exec dev bash
# I'm now inside the container
npm i -g @bazel/bazelisk
bazel run vscode:packager /usr/src/app/dataform.vsix

I'm attaching the compiled .vsix to this comment (you need to unzip it first): dataform.vsix.zip.

I also manually tested the extension on my own machine after installing the compiled .vsix (although it's always possible there was an edge case I didn't catch).

(By the way, I accidentally pushed the docker-compose workaround to the branch this PR is requesting to merge. I reverted this commit, since I'm not proposing that this workaround be incorporated into the upstream repo.)

@GJMcGowan
Copy link
Collaborator

Great! I actually built this locally and it worked fine. I will merge now and do a new release. Thanks!

@GJMcGowan GJMcGowan merged commit 6560ab8 into dataform-co:main Jan 3, 2024
2 checks passed
@barna-emarsys
Copy link
Contributor Author

I've submitted a new PR (#1639) with the 2 new configuration options I've mentioned on this thread. (I used my personal account.)

@barna-emarsys barna-emarsys deleted the vscode-completion-bug branch January 7, 2024 17:56
moker-spaghetti pushed a commit to moker-spaghetti/dataform that referenced this pull request May 26, 2024
* avoid crashes on code completion requests

* workaround for building on arm mac

* Revert "workaround for building on arm mac"

This reverts commit dea8639.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants