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

Feature comment docs onhover #234

Merged
merged 12 commits into from
May 15, 2020

Conversation

nikita-skobov
Copy link
Contributor

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.

@codecov
Copy link

codecov bot commented May 14, 2020

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.

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:

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

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 [email protected]: The platform "linux" is incompatible with this module.
info "[email protected]" is an optional dependency and failed compatibility check. Excluding it from installation.
info [email protected]: The platform "linux" is incompatible with this module.
info "[email protected]" 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 [email protected]: The engine "vscode" appears to be invalid.
[2/5] Resolving packages...
[3/5] Fetching packages...
warning [email protected]: 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

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:

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

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?

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
Copy link
Contributor Author

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

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.

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 the tests and your contribution! I've added a bunch of comments that I hope you find useful.

/**
* Find a block of comments above a line position
*/
public commentsAbove(uri: string, line: number): string | null {
Copy link
Collaborator

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.

@nikita-skobov
Copy link
Contributor Author

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.

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
akurtakov added a commit to akurtakov/shellwax that referenced this pull request May 27, 2020
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 <[email protected]>
akurtakov added a commit to eclipse-shellwax/shellwax that referenced this pull request May 27, 2020
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 <[email protected]>
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