bash-language-server icon indicating copy to clipboard operation
bash-language-server copied to clipboard

Sourcing aware symbols completion and jump to definition

Open skovhus opened this issue 4 years ago • 4 comments

Related #220 (solves everything besides the local scope).

This PR supports:

  • internally resolve all files that are sourced (i.e. . my-file.sh or source ~/something). This is done with a regular expression for now until the grammar supports this natively
  • only do auto-completion and jump to definition for symbols in files that are sourced (previously we completed for all symbols found in the workspace)
  • support jump-to-definition on the file path using in a source command

Before

1

After

2

skovhus avatar May 25 '20 14:05 skovhus

@nikita-skobov let me know if you want to try this out. I would like your feedback.

skovhus avatar May 25 '20 14:05 skovhus

Codecov Report

Merging #244 (f959c12) into main (5b30de3) will increase coverage by 1.39%. The diff coverage is 87.20%.

:exclamation: Current head f959c12 differs from pull request most recent head 458aeb0. Consider uploading reports for the commit 458aeb0 to get more accurate results

@@            Coverage Diff             @@
##             main     #244      +/-   ##
==========================================
+ Coverage   75.29%   76.68%   +1.39%     
==========================================
  Files          19       20       +1     
  Lines         688      755      +67     
  Branches      122      139      +17     
==========================================
+ Hits          518      579      +61     
- Misses        153      155       +2     
- Partials       17       21       +4     
Impacted Files Coverage Δ
vscode-client/src/extension.ts 0.00% <0.00%> (ø)
server/src/server.ts 59.79% <50.00%> (ø)
server/src/util/sourcing.ts 86.48% <86.48%> (ø)
server/src/analyser.ts 85.00% <90.47%> (+0.69%) :arrow_up:
server/src/config.ts 96.15% <100.00%> (+0.69%) :arrow_up:
server/src/linter.ts 90.00% <0.00%> (-1.31%) :arrow_down:
server/src/parser.ts 80.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

codecov[bot] avatar May 25 '20 14:05 codecov[bot]

I'll play around with this more tomorrow, but for now, as an overall impression I have this to say:

Please note I am biased because I created and use a bash transclusion preprocessor which uses an import syntax. Now despite using a preprocessor, this extension, prior to this PR, would still show me onHover, and onCompletion for variables/functions defined in other files. This is/was very convenient to me because I can write a script like:

import * from some_file.sh

some_funct

And as I start typing some_funct, it will give me autocompletion suggestions based on all files in the vscode project directory. But after this PR, this would no longer be the case because it's not a source statement, but rather an import.

Also, in other projects of mine, I have source statements that are dynamic based on other variables, consider:

if [[ -z $SCRIPTPATH ]]; then SCRIPTPATH="$( cd "$(dirname "$0")" >/dev/null 2>&1 ; pwd -P )" ; fi
LIBPATH="$SCRIPTPATH/../../lib/lib.sh"
if [[ -z $__COMPLETION_LIB_LOADED ]]; then source "$LIBPATH" ; fi

Even though the lib.sh file is within the same repository, and contains functions that are used by many scripts in this project, this PR would prevent those functions from getting autocompletion/onhover.

I realize that many people use this project, and I don't want my bias from getting in the way of a good feature being added

Some ideas to consider before merging this PR:

  • maybe get feedback from other users based on their usage of this extension (idk how we'd ask for feedback other than maybe pinning something on the README :woman_shrugging: )
  • maybe modify the contributes.configuration like you did with https://github.com/bash-lsp/bash-language-server/blob/c7d48697c048001477c7d61ad72490fe3652f980/vscode-client/package.json#L34 and add an option to disable sourceAware
  • in addition to an option to disable sourceAware, maybe add a sourcePattern which will contain keywords like .,source and users can add other keywords (eg: I would want to add import), and then the SOURCED_FILES_REG_EXP in util/sourcing.ts would need to be created dynamically based on the value read from the users configuration.

Lastly, I just have a question: is this an optimization? Is it for performance, or user experience?

In the gif posted above it seems you are showing an example where you have to scroll quite a bit until you find the variables you are looking for. Idk about others, but my experience with code completion is I usually start typing the variable/function I want to use, and if it doesnt show up right away, I type another character. Usually after 2 or 3 characters there's only one or two left in the list, and I can hit enter.

(PS. How do you make gifs of code? I've done so before and It envolved screen recording -> ffmpeg -> convert to gif which is a long process. Is there a good tool that lets you easily take gifs of your code?)

nikita-skobov avatar May 26 '20 01:05 nikita-skobov

Thank you so much for your very valuable feedback! 🙌

Lastly, I just have a question: is this an optimization? Is it for performance, or user experience?

My primary concern with this project (and most software I write) is the user experience. :) I want the server to be as useful as possible... and I thought that completing on all symbols in the workspace to be annoying for some users.

But my challenge with this project is that I only use bash for smaller scripts (so the current implementation where we return all workspace symbols works for me).

You raised a lot of great points here: dynamic imports, usage of preprocessors (tbh I was not aware of these), custom import keywords.

In the gif posted above it seems you are showing an example where you have to scroll quite a bit until you find the variables you are looking for

The point was that a lot of the suggestions are irrelevant and not in scope.

How do you make gifs of code?

licecap but there are also some alternatives

skovhus avatar May 26 '20 08:05 skovhus