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

adding zsh expansion flags #115

Merged
merged 1 commit into from
Dec 15, 2021
Merged

Conversation

ryaminal
Copy link
Contributor

@ryaminal ryaminal commented Nov 8, 2021

See https://zsh.sourceforge.io/Doc/Release/Expansion.html#Parameter-Expansion-Flags for more information on this

Hi all,

I'm wanting to add this feature that I understand is ZSH specific. I understand that this may not be desirable to have in the bash tree-sitter, but I figured I'd start the conversation with these changes and see what people's thoughts are.

I've added zsh to the package.json's file-types section. This can, likely, be removed and a user can add a used_by section to the config.

I've tested this with nvim and tree-sitter highlight(along with test, of course).

@ryaminal ryaminal mentioned this pull request Dec 1, 2021
@ryaminal
Copy link
Contributor Author

ryaminal commented Dec 1, 2021

I've been using this for a month now and it makes most stuff in zsh pretty usable.

@ryaminal
Copy link
Contributor Author

Hi there. Not certain on etiquette here. Wondering if someone can have a gander at this?

@skovhus
Copy link
Contributor

skovhus commented Dec 15, 2021

It would be awesome if zsh was supported 👏

@maxbrunsfeld

@maxbrunsfeld maxbrunsfeld merged commit 275effd into tree-sitter:master Dec 15, 2021
@maxbrunsfeld
Copy link
Contributor

This makes sense. Thanks!

@skovhus
Copy link
Contributor

skovhus commented Dec 18, 2021

@maxbrunsfeld thanks. Will you cut a new release?

@verhovsky
Copy link
Collaborator

verhovsky commented Mar 27, 2022

I don't like this. I want to parse bash, not zsh. You should fork this into a separate tree-sitter-zsh repo instead of making everyone accept zsh syntax.

@xPMo
Copy link

xPMo commented Aug 22, 2022

I agree with @verhovsky, while this is a good first start, there's a lot more that would be needed to actually support Zsh properly, and that would make sense in a fork.

For Zsh expansion flags alone, this PR incorrectly parses ${(s:):)foo} and ${(j(:)P)bar}. Support for subscript flags, globbing flags, short_loops, setopt, and more are desired.

@verhovsky
Copy link
Collaborator

I reverted this change for the reason I mentioned above. We could even create a separate tree-sitter-shell repository that parses (tries to parse) all shells.

@vxe
Copy link

vxe commented Aug 10, 2023

@ryaminal any interest in getting the fork going?

@ryaminal
Copy link
Contributor Author

Bleh, when this was reverted last year, unilaterally, after it was in the code for almost a year, I was pretty frustrated. Still frustrated.

That being said, I avoided a fork because the overhead there for a few small changes seemed not worth it. It's 90% the same, so why not add a few pieces. We can surely create a generic tree-sitter-shell repo, but can't this repo be the generic one?

@xPMo
Copy link

xPMo commented Aug 18, 2023

We can surely create a generic tree-sitter-shell repo

I'm not so sure. Bash and Zsh differ syntactically. Attempting to support both at once will cause bugs, like these from Zsh's relaxed brace rules:

Syntax Zsh Bash
$#word ${#word} ${#}word
$a[1] ${a[1]} ${a}[1]
{ echo } } closes the sublist } is an argument to echo, sublist is still open

@xPMo
Copy link

xPMo commented Aug 18, 2023

<sidenote>
Parsing Zsh reliably is nigh-impossible. That behavior I just mentioned? Yeah, you can disable it during runtime and make it parse like Ksh and Bash does. In fact:

f(){
    x=y
    echo $#x
}
f            # prints "1"
emulate ksh  # shortcut for an option set which, among others, includes "ksh_arrays" and "posix_identifiers"
f            # prints "0x"

<sidesidenote>For this reason, if you're writing a Zsh plugin, any function which is an entrypoint from outside your code should have emulate -L zsh as its first line to reset the option set -Locally.</sidesidenote>
</sidenote>

This is uncommon though. In any case, if someone wants to use Ksh emulation, they should probably use a Ksh or Bash parser anyway.

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.

6 participants