Skip to content

feat: support leading dynamic section when sourcing #1086

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 1 commit into from
Apr 13, 2025

Conversation

aarondill
Copy link
Contributor

This does the same thing that ShellCheck does (here).

To support the common pattern of . "$CONFIGDIR/mylib.sh", ShellCheck strips one leading, dynamic section before trying to locate the rest.

fixes #926, fixes #659

@aarondill aarondill marked this pull request as draft January 21, 2024 12:18
@aarondill
Copy link
Contributor Author

this can now handle all the below cases:
. "$dir/file"
. "$dir/other/file"
. "$dir"/file
. $dir/file
. $dir"/file"
. "$dir"/"file"
. "${dir:-.}"/"file"
. ${dir}/"fi"l'e'
source "$dir/file"
It intentionally fails on these:
. "$dir/$file"
. "$dir/other/$file"
. "$(cmd)/file"
. $(cmd)/file
Tests are coming soon! (once i figure out this testing framework)

@Shane-XB-Qian
Copy link
Contributor

It intentionally fails on these:

is there a way to make it work? seems that's the much more usage case.

@aarondill
Copy link
Contributor Author

It intentionally fails on these:

is there a way to make it work? seems that's the much more usage case.

potentially, but these cases are process substitution and dynamic file sourcing. if you really want to use process substitution, assign it to a variable, then use that - this way it will work with shellcheck too. as to dynamic file sourcing, at the point this is evaluated, there's no way to get the "static" value of a variable, so there's no meaningful way to support that.

the current implementation matches shellcheck's behavior

@aarondill aarondill marked this pull request as ready for review February 6, 2024 14:47
@Shane-XB-Qian
Copy link
Contributor

Shane-XB-Qian commented Feb 6, 2024

there's no way

🤔 ok, anyway, this looks was an good improvement.

@isc-aray
Copy link

isc-aray commented Jun 3, 2024

This would be great! All of our sources are of the form . "$ScriptDir/something.sh" or . "$ScriptDir/dir/something.sh".

@skovhus
Copy link
Collaborator

skovhus commented Apr 13, 2025

Sorry for not getting back to you before, I'm not actively maintaining this project TBH. But the change here looks really useful.

@skovhus
Copy link
Collaborator

skovhus commented Apr 13, 2025

@aarondill seems to one failing case, do you mind having a look?

This does the same thing that ShellCheck does ([here](https://github.com/koalaman/shellcheck/blob/ba86c6363c30a5dbefd0b8b9a7c5f4ab0478dc91/src/ShellCheck/Parser.hs#L2277-L2283)).

`To support the common pattern of . "$CONFIGDIR/mylib.sh", ShellCheck strips one leading, dynamic section before trying to locate the rest.`

fixes bash-lsp#926, fixes bash-lsp#659

fix: handle leading dynamic source paths when concatenating

This fixes `${var}/path` and `"${var}"/path` as well as countless variations of these patterns

Note: this turned out to be rather complex task, so it's broken out into a seperate function, which allows for early returns

refactor: move resolveStaticString to util/tree-sitter.ts

This seemed the most appropriate place for it.

tests: fix existing snapshot and add test for dynamic source paths

Adds a simple test for dynamic source paths and updates the snapshot to match new tests / behaivor.

Note: all the line numbers in the snapshot increased by 2 because I added two new lines (the test and a blank one)

tests: add new test in fixtures/sourcing.sh

I added this to the bottom of the file to minimize snapshot diffs. they're large enough as it is.
@aarondill
Copy link
Contributor Author

I squashed it (mostly to make rebasing easier) and rebased to main. After adding a check to avoid a regression of f8b202b, it passes all the tests!

Copy link

codecov bot commented Apr 13, 2025

Codecov Report

Attention: Patch coverage is 74.50980% with 13 lines in your changes missing coverage. Please review.

Project coverage is 80.63%. Comparing base (cd62a5b) to head (a79f2af).
Report is 15 commits behind head on main.

Files with missing lines Patch % Lines
server/src/util/sourcing.ts 67.74% 10 Missing ⚠️
server/src/util/tree-sitter.ts 85.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1086      +/-   ##
==========================================
- Coverage   80.91%   80.63%   -0.29%     
==========================================
  Files          29       29              
  Lines        1457     1503      +46     
  Branches      353      371      +18     
==========================================
+ Hits         1179     1212      +33     
- Misses        221      233      +12     
- Partials       57       58       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@skovhus skovhus merged commit eda36e2 into bash-lsp:main Apr 13, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants