-
Notifications
You must be signed in to change notification settings - Fork 137
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
Conversation
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
It intentionally fails on these:
is there a way to make it work? seems that's the much more usage case.
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
1ae72d3 to
2d19b5d
Compare
there's no way
🤔 ok, anyway, this looks was an good improvement.
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".
Sorry for not getting back to you before, I'm not actively maintaining this project TBH. But the change here looks really useful.
@aarondill seems to one failing case, do you mind having a look?
16b7068 to
4edd2a9
Compare
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.
4edd2a9 to
a79f2af
Compare
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!
Codecov ReportAttention: Patch coverage is
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. 🚀 New features to boost your workflow:
|
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