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

Upgrade dependencies #1148

Merged
merged 4 commits into from
Apr 23, 2024
Merged

Upgrade dependencies #1148

merged 4 commits into from
Apr 23, 2024

Conversation

Jimmyscene
Copy link
Contributor

Upgrade web-tree-sitter, tree-sitter-cli and tree-sitter-bash

These sort of all need to be done at the same time else everything blows up

Update rootNode.hasError() to hasError per
https://github.com/tree-sitter/tree-sitter/pull/3103/files

Update build-wasm flag to build --wasm per
tree-sitter/tree-sitter@99b93d8

Copy link

codecov bot commented Apr 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.52%. Comparing base (8f06a20) to head (f8b202b).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1148      +/-   ##
==========================================
+ Coverage   81.50%   81.52%   +0.02%     
==========================================
  Files          28       28              
  Lines        1346     1348       +2     
  Branches      319      320       +1     
==========================================
+ Hits         1097     1099       +2     
  Misses        204      204              
  Partials       45       45              

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

Blake Easley added 2 commits April 21, 2024 17:14
Upgrade web-tree-sitter, tree-sitter-cli and tree-sitter-bash

These sort of all need to be done at the same time else everything blows
up

Update rootNode.hasError() to hasError per
https://github.com/tree-sitter/tree-sitter/pull/3103/files

Update build-wasm flag to build --wasm per
tree-sitter/tree-sitter@99b93d8
These are invalid function names, and the updated treesitter grammar
will not parse it
@@ -109,13 +109,15 @@ function getSourcedPathInfoFromNode({
}

if (argumentNode.type === 'string' || argumentNode.type === 'raw_string') {
if (argumentNode.namedChildren.length === 0) {
const children = argumentNode.namedChildren
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when argumentNode.text == 'source "./issue206.sh"', argumentNode.namedChildren contains an element that is string_content with text=='"./issue206.sh"'

I am no expert here, so while this does fix it, there may be a better/more robust way to handle this

@@ -1740,7 +1740,7 @@ describe('server', () => {
describe('Edge or not covered cases', () => {
it('only includes variables typed as variable_name', async () => {
const iRanges = await getFirstChangeRanges(getRenameRequestResult(106, 4))
// This should be 6 if all instances within let and arithmetic
// This should be 6 if all instances within let and c-style for loop
Copy link
Contributor Author

@Jimmyscene Jimmyscene Apr 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I note in the commit message,

The anazlyer is now detecting variables within arithmetic expressions,
but still not let, postfix, or binary expressions

So a rename on

for i in 1 2 3; do
	echo "$i"
done

let i=0
for (( ; i < 10; i++)); do
	echo "$((2 * i))"
done

Would be changed to

for new_name in 1 2 3; do
	echo "$new_name"
done

let i=0
for (( ; i < 10; i++)); do
	echo "$((2 * new_name))"
done

Which is definitely wrong, but I'm interested to hear opinions from the maintainers on this. We already have a test checking that treesitter can't adequately rename these

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mcecode what do you think about this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is to be expected since, as far as I can tell, tree-sitter-bash only updated the types of variables from word to variable_name within arithmetic expressions. For those other constructs they're still typed as word. The current implementation of rename symbol only renames variables typed as variable_name.

Blake Easley added 2 commits April 21, 2024 18:14
The anazlyer is now detecting variables within arithmetic expressions,
but still not let, postfix, or binary expressions

So a rename on

```
for i in 1 2 3; do
	echo "$i"
done

let i=0
for (( ; i < 10; i++)); do
	echo "$((2 * i))"
done
```

Would be changed to

```
for new_name in 1 2 3; do
	echo "$new_name"
done

let i=0
for (( ; i < 10; i++)); do
	echo "$((2 * new_name))"
done
```
when argumentNode.text == 'source "./issue206.sh"',
argumentNode.namedChildren contains an element that is string_content
with text=='"./issue206.sh"'
@Jimmyscene Jimmyscene marked this pull request as ready for review April 22, 2024 01:23
@Jimmyscene Jimmyscene requested a review from skovhus April 22, 2024 18:29
@mcecode
Copy link
Contributor

mcecode commented Apr 23, 2024

Since I was tagged, I'll leave some thoughts/notes here.

If/once this gets merged, #1134 can be closed as per my comment there #1134 (comment).

#943 can also be closed since that's just urging us to upgrade. On that note, I think tree-sitter-bash can be installed as is from the npm registry now instead of the latest from GitHub since they've started shipping stable versions there again, the latest being 0.21.

@skovhus skovhus merged commit 89f58b8 into bash-lsp:main Apr 23, 2024
6 checks passed
@skovhus
Copy link
Collaborator

skovhus commented Apr 23, 2024

Thanks @Jimmyscene – and thank you @mcecode for adding valuable comments.

@Jimmyscene
Copy link
Contributor Author

Jimmyscene commented Apr 23, 2024

Thanks @Jimmyscene – and thank you @mcecode for adding valuable comments.

Thanks for merging @skovhus - It looks like the Personal Access Token is expired per: https://github.com/bash-lsp/bash-language-server/actions/runs/8806526335/job/24171555526#step:6:116

Edit. Link failed, but it's the Deploy VSCode Extension job

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.

3 participants