Skip to content

chore(style,grammar): remove indirection for punctuation - #236

Merged
fdncred merged 6 commits into
nushell:mainfrom
mkatychev:chore/remove-punctuation-indirection
Dec 6, 2025
Merged

chore(style,grammar): remove indirection for punctuation#236
fdncred merged 6 commits into
nushell:mainfrom
mkatychev:chore/remove-punctuation-indirection

Conversation

@mkatychev

@mkatychev mkatychev commented Dec 4, 2025

Copy link
Copy Markdown
Contributor

Most tree-sitter styles do not use indirection for anonymous nodes as it impacts readability:
https://github.com/tree-sitter/tree-sitter-rust/blob/261b20226c04ef601adbdf185a800512a5f66291/grammar.js#L249-L262

This PR is a style proposal so discussion is welcome/expected

@blindFS

blindFS commented Dec 5, 2025

Copy link
Copy Markdown
Contributor

I'm cool with this change, should we ask for the opinion of the original author?

@fdncred

fdncred commented Dec 5, 2025

Copy link
Copy Markdown
Contributor

I'm good with it. Is the CI broke due to the npm thing or something else?

@mkatychev

Copy link
Copy Markdown
Contributor Author

I can do a follow up doing the same with brackets and other glyphs, this PR was to test the waters.

@mkatychev

mkatychev commented Dec 5, 2025

Copy link
Copy Markdown
Contributor Author

I'm good with it. Is the CI broke due to the npm thing or something else?

It's the NPM thing :\ https://github.com/nushell/tree-sitter-nu/actions/runs/19937120301/job/57165001540?pr=236#step:5:861: https://github.com/nushell/tree-sitter-nu/actions/runs/19937120301/job/57165001540?pr=236#step:5:861

EDIT: I can do the patch here before merge @fdncred

@mkatychev

Copy link
Copy Markdown
Contributor Author

@fdncred should be ready for re-review

Comment thread grammar.js
Comment on lines 1306 to 1307
const func = immediate ? token.immediate : token;
const excluded = '\\[\\]\\-{}<>="`\'@?,:.';

@mkatychev mkatychev Dec 5, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@blindFS @fdncred Just want to call out that these kind of const declarations are a tree-sitter DSL antipattern.

These decls bypass a lot of potential optimizations by inlining where it's not needed.

EDIT: that may only apply to extras 🤔

@blindFS blindFS left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

@fdncred
fdncred merged commit 64ba3f8 into nushell:main Dec 6, 2025
4 checks passed
@fdncred

fdncred commented Dec 6, 2025

Copy link
Copy Markdown
Contributor

Thanks

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