Feature comment docs onhover #234

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

Jump to bottom

Merged

skovhus merged 12 commits into bash-lsp:master from nikita-skobov:feature-comment-docs-onhover

May 15, 2020

Merged

Feature comment docs onhover #234

skovhus merged 12 commits into bash-lsp:master from nikita-skobov:feature-comment-docs-onhover

May 15, 2020

Conversation

nikita-skobov

Copy link

Contributor

@nikita-skobov nikita-skobov commented May 14, 2020

In most languages, comments above a function serve as documentation for that function. I modified this extension to be able to view the comments that are above functions as tooltips onHover.

It looks like this:

# this will not be included because
# there is an empty line below
# single positional argument: the commit Id
# returns true if that commit id
# is a merge commit
is_merge_commit() {
 # does something based on 1ドル
 if [[ 1ドル == "something" ]]; then
 return 0
 fi
}

defined_this_file

Also works for functions defined in other files that are included in the this.uriToDeclarations from analyser.ts:

# fn_exists.sh
# does a function exist in this script?
# args: 1ドル the name of the function
fn_exists() {
 if [[ 1ドル == "exists" ]]; then
 return 0
 fi
}
# end of fn_exists.sh
# main_script.sh
fn_exists "my_func"

defined_other_file

I followed the steps in the development guide before editing the extension and I got this failure:

 FAIL server/src/__tests__/server.test.ts
 ●くろまる server › responds to onCompletion with filtered list when word is found

so that error is unrelated to my changes. I then ran yarn test again after making my changes, and no other tests failed.

I'm new to working on VScode extensions, so let me know if there is a more idiomatic/efficient way to achieve what my code does.

nikita-skobov added 2 commits

May 13, 2020 19:16

@nikita-skobov


 Add feature to show comment documentation onHover

6dc709e

@nikita-skobov


 Follow style guide

68a15ff

@codecov

Copy link

codecov bot commented May 14, 2020 •

edited

Loading

Codecov Report

Merging #234 into master will increase coverage by 1.86%.
The diff coverage is 94.73%.

Impacted file tree graph

@@ Coverage Diff @@
## master #234 +/- ##
==========================================
+ Coverage 71.61% 73.47% +1.86% 
==========================================
 Files 18 18 
 Lines 539 558 +19 
 Branches 87 90 +3 
==========================================
+ Hits 386 410 +24 
+ Misses 131 125 -6 
- Partials 22 23 +1

Impacted Files	Coverage Δ
server/src/server.ts	`60.50% <66.66%> (+3.36%)`	⬆️
server/src/analyser.ts	`82.82% <100.00%> (+2.54%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d03a676...1e35af1. Read the comment docs.

skovhus

skovhus reviewed

May 14, 2020

View reviewed changes

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.

Thanks! This is a great suggestion.

A few things:

can you add some unit tests documenting this new bahaviour (see https://github.com/nikita-skobov/bash-language-server/blob/68a15ff3c7a578ecf951e1c5754065d745020b67/server/src/__tests__/server.test.ts#L50) and probably also one for the analyzer extension you did
we already have an AST of the program analyzed by tree sitter bash. We would need to figure out if that can return the comment instead of us manually parsing the file. It might not be able to do this right now, but it could be a future improvement
once we have some tests in place I might have a few suggestions around the implementation

Again, thank for your contribution. This would be a great extension!

@skovhus

Copy link

Collaborator

skovhus commented May 14, 2020

I’m curious to know why the tests are failing locally for you. As you see CI isn’t failing, all tests pass. So it must be something in your environment that the test suite is not catering for. Can you share more information/output from the failure. Is there more output from when running the tests? Which OS are you running?

@nikita-skobov

Copy link

Contributor Author

nikita-skobov commented May 14, 2020

I'm a little confused on the architecture with the server. Before I run the tests do I need to do something to set up the server? In any case here is my system information:

OS/distro: Arch linux
node --version: v13.12.0
npm --version: 6.14.4
yarn --version: 1.22.4
vscodium --version: 1.45.0 x64
(I use vscodium instead of vscode maybe that has something to do with it)

I recloned the repo in a new directory straight from master, and I did the following steps:

git clone https://github.com/bash-lsp/bash-language-server
cd bash-language-server
yarn install
# output from yarn install:
yarn install v1.22.4
[1/4] Resolving packages...
[2/4] Fetching packages...
info fsevents@1.2.11: The platform "linux" is incompatible with this module.
info "fsevents@1.2.11" is an optional dependency and failed compatibility check. Excluding it from installation.
info fsevents@2.1.2: The platform "linux" is incompatible with this module.
info "fsevents@2.1.2" is an optional dependency and failed compatibility check. Excluding it from installation.
[3/4] Linking dependencies...
[4/4] Building fresh packages...
$ cd server && yarn install && cd ../vscode-client && yarn install && cd ..
yarn install v1.22.4
[1/5] Validating package.json...
[2/5] Resolving packages...
[3/5] Fetching packages...
[4/5] Linking dependencies...
[5/5] Building fresh packages...
Done in 1.40s.
yarn install v1.22.4
[1/5] Validating package.json...
warning bash-ide-vscode@1.9.1: The engine "vscode" appears to be invalid.
[2/5] Resolving packages...
[3/5] Fetching packages...
warning vscode-languageclient@6.1.3: The engine "vscode" appears to be invalid.
[4/5] Linking dependencies...
[5/5] Building fresh packages...
Done in 1.24s.
Done in 9.04s.
yarn run compile
# output from yarn run compile:
yarn run v1.22.4
$ tsc -b
Done in 9.76s.
yarn run lint
# output from yarn run lint:
yarn run v1.22.4
$ yarn lint:bail --fix
$ eslint . --ext js,ts,tsx --cache --fix
Done in 4.11s.
yarn run test
# output from yarn run test:
yarn run v1.22.4
$ jest --runInBand
 PASS server/src/util/__tests__/sh.test.ts
 FAIL server/src/__tests__/server.test.ts
 ●くろまる server › responds to onCompletion with filtered list when word is found
 expect(received).toBe(expected) // Object.is equality
 Expected: true
 Received: false
 226 | 
 227 | // Limited set (not using snapshot due to different executables on CI and locally)
 > 228 | expect(result && 'length' in result && result.length < 5).toBe(true)
 | ^
 229 | expect(result).toEqual(
 230 | expect.arrayContaining([
 231 | {
 at server/src/__tests__/server.test.ts:228:63
 at fulfilled (server/src/__tests__/server.test.ts:5:58)
 PASS server/src/__tests__/analyzer.test.ts
 PASS server/src/__tests__/config.test.ts
 PASS server/src/__tests__/executables.test.ts
 PASS server/src/util/__tests__/shebang.test.ts
 PASS server/src/util/__tests__/flatten.test.ts
 PASS server/src/util/__tests__/array.test.ts
Test Suites: 1 failed, 7 passed, 8 total
Tests: 1 failed, 1 skipped, 52 passed, 54 total
Snapshots: 18 passed, 18 total
Time: 7.746s
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.

I hope that helps. Let me know if theres any other info needed

@nikita-skobov

Copy link

Contributor Author

nikita-skobov commented May 14, 2020

In regards to the feature PR, I will try my hand at writing some tests. I suppose the tests I need to write/modify are:

modify the onHover test: https://github.com/nikita-skobov/bash-language-server/blob/68a15ff3c7a578ecf951e1c5754065d745020b67/server/src/__tests__/server.test.ts#L50
add a new unit test for the function: commentsAbove in analyser.ts

I can see (somewhat) how to modify the existing test to test for the comment doc feature, but Im not sure I'd know how to go about creating a new test for the commentsAbove function.

In regards to the AST, can you give me some suggestions on how to use it to extract comments at a specific line?

@skovhus

Copy link

Collaborator

skovhus commented May 14, 2020

Can I get you to do a console.log(result) just before the failing assertion and paste the result here. But it is 99% something in your local environment that we are not catering for.

As for the tests, then I can come with some suggestions for these tests if you are fine with that. We basically want to test the different cases (one comment above, multiple comments, no comments) and see how the function reacts to this. After that we can refactor the function and be sure it still works.

About the AST, then it currently doesn’t support this (I investigated it). So your current approach is fine.

@nikita-skobov

Copy link

Contributor Author

nikita-skobov commented May 14, 2020

Yes. Here is the output:

yarn run v1.22.4
$ jest --runInBand
 FAIL server/src/__tests__/server.test.ts
 ●くろまる Console
 console.log server/src/__tests__/server.test.ts:227
 [
 { label: 'rm', kind: 12, data: { name: 'rm', type: 1 } },
 { label: 'rm_pulse', kind: 12, data: { name: 'rm_pulse', type: 1 } },
 { label: 'rmdir', kind: 12, data: { name: 'rmdir', type: 1 } },
 { label: 'rmid', kind: 12, data: { name: 'rmid', type: 1 } },
 {
 label: 'rmiregistry',
 kind: 12,
 data: { name: 'rmiregistry', type: 1 }
 }
 ]
 ●くろまる server › responds to onCompletion with filtered list when word is found
 expect(received).toBe(expected) // Object.is equality
 Expected: true
 Received: false
 228 | 
 229 | // Limited set (not using snapshot due to different executables on CI and locally)
 > 230 | expect(result && 'length' in result && result.length < 5).toBe(true)
 | ^
 231 | expect(result).toEqual(
 232 | expect.arrayContaining([
 233 | {
 at server/src/__tests__/server.test.ts:230:63
 at fulfilled (server/src/__tests__/server.test.ts:5:58)
 PASS server/src/util/__tests__/sh.test.ts
 PASS server/src/__tests__/analyzer.test.ts
 PASS server/src/__tests__/config.test.ts
 PASS server/src/__tests__/executables.test.ts
 PASS server/src/util/__tests__/shebang.test.ts
 PASS server/src/util/__tests__/flatten.test.ts
 PASS server/src/util/__tests__/array.test.ts
Test Suites: 1 failed, 7 passed, 8 total
Tests: 1 failed, 1 skipped, 52 passed, 54 total
Snapshots: 18 passed, 18 total
Time: 6.37s, estimated 7s
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.

Maybe Im wrong but it seems the issue is that the test assumes that there are less than 5 executables on a user's PATH that begin with rm, whereas my system has 5. so the result.length < 5 expectation fails

Solution: update to less than 6? Or otherwise it probably doesn't matter much because this test fails based on a user's path, and as long as it works on the CI, does it really matter if a user has a bunch of extra executables that start with rm?

nikita-skobov added 4 commits

May 14, 2020 11:42

@nikita-skobov


 Adds testing fixture comment-doc-on-hover.sh

95f7f2d

@nikita-skobov


 Updates FIXTURE_FILES_MATCHING_GLOB

5938cc1

previously it was hardcoded at 10. It took me a while to realize that 10 referred to the number of files in the fixtures folder that ended in .sh and since I added another file: comment-doc-on-hover.sh this value of 10 needed to be updated to 11

@nikita-skobov


 Updates onHover test with case for comment doc feature

a7ccdd3

@nikita-skobov


 Adds test for commentsAbove in analyser.ts

55643e7

@nikita-skobov

Copy link

Contributor Author

nikita-skobov commented May 14, 2020

I added the tests mentioned above using a new fixture: comment-doc-on-hover.sh which has some different examples of comments being shown on the tooltip.

Also, I wonder whats the best way to display the comments, ie: should it preserve newlines or not? Should it attempt to style it differently somehow?

The current functionality will not preserve comment newlines, eg:

# this is
# a comment
my_func() { }

will be returned from commentsAbove as:

 this is\n a comment

but because it's rendered with markdown it will show up as:

 this is a comment

Personally I think this is fine because comment documentations are typically wrapped around when the line gets too long, and this allows you to read it as a sentence like it was intended.

But that's just my opinion, changing it to preserve the newlines would be trivial: just add an extra \n when it joins the lines.

@skovhus

Copy link

Collaborator

skovhus commented May 14, 2020 •

edited

Loading

Solution: update to less than 6? Or otherwise it probably doesn't matter much because this test fails based on a user's path

Let us do less than 8. Would be nice that it runs on your machine. The point of the test is just that the set returned is limited.

skovhus

skovhus reviewed

May 14, 2020

View reviewed changes

Copy link

Collaborator

@skovhus skovhus left a comment •

edited

Loading

Choose a reason for hiding this comment

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

Thank you so much for the tests and your contribution! I've added a bunch of comments that I hope you find useful.

server/src/__tests__/analyzer.test.ts Outdated Show resolved Hide resolved

server/src/analyser.ts Outdated Show resolved Hide resolved

server/src/__tests__/analyzer.test.ts Outdated Show resolved Hide resolved

server/src/__tests__/server.test.ts Outdated Show resolved Hide resolved

server/src/analyser.ts

/**

* Find a block of comments above a line position

public commentsAbove(uri: string, line: number): string | null {

Copy link

Collaborator

@skovhus skovhus May 14, 2020

Choose a reason for hiding this comment

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

I'm wondering if we should call this findRelatedComment. More precise as it only returns a comment for when we find a match above the function/variable.

server/src/analyser.ts Outdated Show resolved Hide resolved

server/src/server.ts Outdated Show resolved Hide resolved

testing/fixtures/comment-doc-on-hover.sh Show resolved Hide resolved

nikita-skobov added 6 commits

May 14, 2020 16:57

@nikita-skobov


 Changes result set length to 8

35b49bf

maybe a user has several executables on their path that start with 'rm', so for this test to be a bit more inclusive, we increase the result.length maximum

@nikita-skobov


 Updates commentsAbove to strip leading whitespace

dae023f

@nikita-skobov


 Adds more test cases for commentsAbove in analyzer

fb307c2

@nikita-skobov


 Follows style guide

8905da4

@nikita-skobov


 Adds helper function to onHover for comment-doc formatting

0b3210a

@nikita-skobov


 Uses seperate test case for onHover documentation

1e35af1

@nikita-skobov

Copy link

Contributor Author

nikita-skobov commented May 14, 2020

As of commit 1e35af1 I think Ive resolved all issues mentioned in the comments. Please let me know if this can be merged into master, or if there are any other issues.

skovhus

skovhus approved these changes

May 15, 2020

View reviewed changes

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.

Thank you so much for your contribution. I'll release a new version with this (soonish).

@skovhus skovhus merged commit 2e02f9b into bash-lsp:master

May 15, 2020

@skovhus skovhus mentioned this pull request

May 15, 2020

Use comments as symbol documentation #235

Merged

akurtakov added a commit to akurtakov/shellwax that referenced this pull request

May 27, 2020

@akurtakov


 Update to Bash LS 1.16.1

e6ff77d

Language server changelog:
1.16.1
 Fix brace expansion bug
(bash-lsp/bash-language-server#240)
 Do not crash if bash is not installed
(bash-lsp/bash-language-server#242)
1.16.0
 Improved completion handler for parameter expansions
(bash-lsp/bash-language-server#237)
1.15.0
 Use comments above symbols for documentation
(bash-lsp/bash-language-server#234,
bash-lsp/bash-language-server#235)
1.14.0
 onHover and onCompletion documentation improvements
(bash-lsp/bash-language-server#230)
 support 0/1 as values for HIGHLIGHT_PARSING_ERRORS
(bash-lsp/bash-language-server#231)
1.13.1
 Gracefully handle glob failures
(bash-lsp/bash-language-server#224,
bash-lsp/bash-language-server#226)
 Maintenance
(bash-lsp/bash-language-server#222,
bash-lsp/bash-language-server#225)
Signed-off-by: Alexander Kurtakov <akurtako@redhat.com>

@akurtakov akurtakov mentioned this pull request

May 27, 2020

Update to Bash LS 1.16.1 eclipse-shellwax/shellwax#46

Merged

akurtakov added a commit to eclipse-shellwax/shellwax that referenced this pull request

May 27, 2020

@akurtakov


 Update to Bash LS 1.16.1

d2a6972

Language server changelog:
1.16.1
 Fix brace expansion bug
(bash-lsp/bash-language-server#240)
 Do not crash if bash is not installed
(bash-lsp/bash-language-server#242)
1.16.0
 Improved completion handler for parameter expansions
(bash-lsp/bash-language-server#237)
1.15.0
 Use comments above symbols for documentation
(bash-lsp/bash-language-server#234,
bash-lsp/bash-language-server#235)
1.14.0
 onHover and onCompletion documentation improvements
(bash-lsp/bash-language-server#230)
 support 0/1 as values for HIGHLIGHT_PARSING_ERRORS
(bash-lsp/bash-language-server#231)
1.13.1
 Gracefully handle glob failures
(bash-lsp/bash-language-server#224,
bash-lsp/bash-language-server#226)
 Maintenance
(bash-lsp/bash-language-server#222,
bash-lsp/bash-language-server#225)
Signed-off-by: Alexander Kurtakov <akurtako@redhat.com>

Labels

None yet

2 participants

@nikita-skobov @skovhus

Feature comment docs onhover #234

Feature comment docs onhover #234

Uh oh!

Conversation

@nikita-skobov nikita-skobov commented May 14, 2020

Uh oh!

codecov bot commented May 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

@skovhus skovhus left a comment

Choose a reason for hiding this comment

Uh oh!

skovhus commented May 14, 2020

Uh oh!

nikita-skobov commented May 14, 2020

Uh oh!

nikita-skobov commented May 14, 2020

Uh oh!

skovhus commented May 14, 2020

Uh oh!

nikita-skobov commented May 14, 2020

Uh oh!

nikita-skobov commented May 14, 2020

Uh oh!

skovhus commented May 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

@skovhus skovhus left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

@skovhus skovhus May 14, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nikita-skobov commented May 14, 2020

Uh oh!

@skovhus skovhus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented May 14, 2020 •

edited

Loading

skovhus commented May 14, 2020 •

edited

Loading

@skovhus skovhus left a comment •

edited

Loading