Skip to content

Add explainshell integration #45

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

Merged
merged 13 commits into from
Jun 6, 2018

Conversation

chrismwendt
Copy link
Contributor

@chrismwendt chrismwendt commented Jun 4, 2018

2018-06-03 20 59 17

This adds a VS Code setting bash.explainshellEndpoint which is forwarded to the language server as an environment variable EXPLAINSHELL_ENDPOINT (when "" the feature is disabled, and once idank/explainshell#125 is merged in, you'll be able to set this to "https://explainshell.com"). When enabled, HTTP requests will be sent to explainshell upon hover to show documentation for flags.

How to run this

docker run --name bash-explainshell --rm -p 5000:5000 chrismwendt/codeintel-bash-with-explainshell

Then set this in your VS Code config:

    "bashIde.explainshellEndpoint": "http://localhost:5000",

Then launch the version of the extension on this branch through VS Code and you should see docs on hover like in the GIF above.

Resolves #42

@chrismwendt
Copy link
Contributor Author

Any idea why yarn run test errors out and says TypeError: document.getText is not a function? yarn run compile works fine, and there's no type error in VS Code.

@mads-hartmann
Copy link
Collaborator

@chrismwendt This is amazing 🎉 I have a very busy day today but I'll try to squeeze in a review - but this a awesome 👏

Copy link
Collaborator

@skovhus skovhus left a comment

Choose a reason for hiding this comment

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

Great work! I added a few comments.

"type": "object",
"title": "Bash IDE configuration",
"properties": {
"bash.explainshellEndpoint": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let us call this bashIde. instead of bash.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

- [ ] Rename symbol

## Configuration

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess we need some more explanation here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add a link to how to set up explainshell in Docker?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My internet is back up and I pushed the pre-built image https://hub.docker.com/r/chrismwendt/codeintel-bash-with-explainshell/ And I added this to the docs

Copy link
Collaborator

Choose a reason for hiding this comment

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

Awesome. Maybe also a link to idank/explainshell#125 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@chrismwendt
Copy link
Contributor Author

Yes, CI is ✅ @mads-hartmann Anything else?

Copy link
Collaborator

@skovhus skovhus left a comment

Choose a reason for hiding this comment

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

Looks good. 👌 I only have one change request related to the testing/package.json file.

@@ -0,0 +1,5 @@
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should need a package.json file here. The package.json file in the root is where all test dependencies are.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let me know what failed without this. The folder is just meant for testing utilities.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed a data type from string to TextDocument in the tests, so the tests need vscode-languageserver.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That makes sense. But then vscode-languageserver should be added to the root package.json file as a root dependency. That should work. 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But it doesn't - if you check out this branch and remove the dependency:

diff --git a/testing/package.json b/testing/package.json
index aa082ff..18a1e41 100644
--- a/testing/package.json
+++ b/testing/package.json
@@ -1,5 +1,3 @@
 {
-  "dependencies": {
-    "vscode-languageserver": "^4.1.1"
-  }
+  "dependencies": {}
 }

Then run yarn run check:bail:

$ jest --runInBand --forceExit
 FAIL  server/src/__tests__/analyzer.test.ts
  ● Test suite failed to run

    Cannot find module 'vscode-languageserver' from 'fixtures.ts'

      3 | import * as LSP from 'vscode-languageserver'
      4 |
    > 5 | const base = path.join(__dirname, './fixtures/')
      6 |
      7 | const FIXTURES = {
      8 |   INSTALL: LSP.TextDocument.create('foo', 'bar', 0, fs.readFileSync(path.join(base, 'install.sh'), 'utf8')),

      at Resolver.resolveModule (node_modules/jest-resolve/build/index.js:169:17)
      at Object.<anonymous> (testing/fixtures.ts:5:13)

(node:52339) ExperimentalWarning: The fs.promises API is experimental
 PASS  server/src/__tests__/executables.test.ts
 PASS  server/src/__tests__/builtins.test.ts
 PASS  server/src/util/__tests__/flatten.test.ts

Test Suites: 1 failed, 3 passed, 4 total
Tests:       1 skipped, 9 passed, 10 total
Snapshots:   0 total
Time:        1.903s, estimated 2s
Ran all test suites.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

You can't remove the import because it's referenced in testing/fixtures.ts.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But vscode-languageserver is not installed as a devDependency in the root package.json - that is why it fails. Can you try doing that instead of adding it to another package.json file inside the testing folder.

Basically we have a package containing all infrastructure needed to type, lint and test in the root package.json. This should contain additional dependencies needed for testing.

Thanks. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This still fails

diff --git a/package.json b/package.json
index 8680641..c0330f1 100644
--- a/package.json
+++ b/package.json
@@ -24,7 +24,8 @@
     "tslint-config-prettier": "^1.10.0",
     "tslint-plugin-prettier": "^1.3.0",
     "tslint": "^5.9.1",
-    "typescript": "^2.8.1"
+    "typescript": "^2.8.1",
+    "vscode-languageserver": "^4.1.1"
   },
   "jest": {
     "testEnvironment": "node",
diff --git a/testing/package.json b/testing/package.json
index aa082ff..9664f26 100644
--- a/testing/package.json
+++ b/testing/package.json
@@ -1,5 +1,4 @@
 {
   "dependencies": {
-    "vscode-languageserver": "^4.1.1"
   }
 }

Can you make the change you want instead of telling me?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ugh, I didn't run yarn install before yarn run check:bail, it works after all.

- [ ] Rename symbol

## Configuration

Copy link
Collaborator

Choose a reason for hiding this comment

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

Awesome. Maybe also a link to idank/explainshell#125 ?

Copy link
Collaborator

@mads-hartmann mads-hartmann left a comment

Choose a reason for hiding this comment

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

I have no further comments besides the two pending ones already stated by @skovhus.

This is great. I can't wait to try it out.

Copy link
Collaborator

@skovhus skovhus left a comment

Choose a reason for hiding this comment

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

Yay! Thank you so much for the contribution. 🙌

Copy link
Collaborator

@mads-hartmann mads-hartmann left a comment

Choose a reason for hiding this comment

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

Awesome 🎉

@mads-hartmann mads-hartmann merged commit 00941c0 into bash-lsp:master Jun 6, 2018
@mads-hartmann
Copy link
Collaborator

@chrismwendt Merged! 😊 I'll cut a release today. Thanks a lot for the contribution

@chrismwendt chrismwendt deleted the explainshell branch June 6, 2018 10:58
@chrismwendt
Copy link
Contributor Author

Sweet, the new language server is running on Sourcegraph.com now https://sourcegraph.com/github.com/creationix/nvm/-/blob/install.sh#L67:12

image

And you can get this functionality on GitHub (or any supported code host) with the Chrome extension (or other browsers) https://github.com/creationix/nvm/blob/HEAD/install.sh#L60

image

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.

3 participants