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

Sourcing aware symbols (completion + jump to definition) #244

Merged
merged 13 commits into from
Dec 9, 2022

Conversation

skovhus
Copy link
Collaborator

@skovhus skovhus commented May 25, 2020

Related #220 (not solving 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 used in a source command
  • disables globally finding symbols (yay no more weird suggestions)
  • the new behavior can be disabled by turning on the includeAllWorkspaceSymbols configuration option.

Before

1

After

2

Update November, 2022

This seems like a nice improvement, and the solution from 2020 has been improved to resolve paths based on the workspace folder and the sourcing file. We also dynamically load files that are sourced so the background analysis is not really needed.

@skovhus
Copy link
Collaborator Author

skovhus commented May 25, 2020

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

@codecov
Copy link

codecov bot commented May 25, 2020

Codecov Report

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

❗ 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%) ⬆️
server/src/config.ts 96.15% <100.00%> (+0.69%) ⬆️
server/src/linter.ts 90.00% <0.00%> (-1.31%) ⬇️
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.

@nikita-skobov
Copy link
Contributor

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 🤷‍♀️ )
  • maybe modify the contributes.configuration like you did with
    "bashIde.globPattern": {

    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?)

@skovhus
Copy link
Collaborator Author

skovhus commented May 26, 2020

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 skovhus marked this pull request as draft May 26, 2020 08:26
@skovhus skovhus mentioned this pull request Jun 2, 2020
skovhus added a commit that referenced this pull request Nov 28, 2022
@skovhus skovhus force-pushed the sourcing-aware-symbols branch from e447dd2 to f959c12 Compare November 28, 2022 12:19
@skovhus skovhus force-pushed the sourcing-aware-symbols branch 5 times, most recently from b22dfb4 to 7de5865 Compare December 8, 2022 14:36
- Only variables found in the sourced files are completed on
- Jump to definition works on the source file path (e.g. clicking "source my-file.sh")
This fixes a small bug in vscode where documentation that pops out to
the right was not formatted using markdown.
@skovhus skovhus force-pushed the sourcing-aware-symbols branch from 6df1739 to dff893f Compare December 9, 2022 08:39
@skovhus skovhus marked this pull request as ready for review December 9, 2022 12:06
@skovhus skovhus changed the title Sourcing aware symbols completion and jump to definition Sourcing aware symbols (completion + jump to definition) Dec 9, 2022
@skovhus skovhus enabled auto-merge December 9, 2022 12:17
@skovhus skovhus merged commit 1788418 into main Dec 9, 2022
@skovhus skovhus deleted the sourcing-aware-symbols branch December 9, 2022 12:17
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