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

Clarify snippet placeholder structure #2032

Open
wants to merge 1 commit into
base: gh-pages
Choose a base branch
from

Conversation

echasnovski
Copy link

Now grammar implies that placeholder value should be a single any node, while in most applications it can be any number of consecutive any nodes. For example, as described in earlier "Placeholders" section: ${1:another ${2:placeholder}}.

Using any+ instead of any seems to be more precise.

@ribru17
Copy link
Contributor

ribru17 commented Dec 5, 2024

Would it make more sense to leave these unchanged, and instead change any to:

(tabstop | placeholder | choice | variable | text)+

currently the grammar technically implies that there can only be one top-level any node in a snippet

@echasnovski
Copy link
Author

Would it make more sense to leave these unchanged, and instead change any to:

(tabstop | placeholder | choice | variable | text)+

currently the grammar technically implies that there can only be one top-level any node in a snippet

Yeah, maybe. This more feels like a style preference. I don't think grammar is a place for implications, so not sure if the first (or the broadest) definition should be treated as top level. But having an explicit mention about top level snippet parts might be useful indeed.

@ribru17
Copy link
Contributor

ribru17 commented Dec 5, 2024

For sure. And just to be clear, either way the grammar right now is incorrect in that it only allows one any node at the top level (even something like text$1 is incorrect right now because it is a sequence of two any nodes). I didn't want to make a PR to change any to (tabstop | placeholder | choice | variable | text)+ because this one seems like it would be the perfect place to do that since it solves two issues with one. That said, we could keep this PR unchanged and then add something like

source      ::= any+

such that now source is the top level node rather than any, but that seems a lot less concise

@echasnovski echasnovski force-pushed the snippet_placeholder-any branch from 39bdf54 to 96ca492 Compare December 6, 2024 11:47
Now grammar implies that placeholder value should be a single `any`
node, while in most applications it can be any number of consecutive
`any` nodes. For example, as described in earlier "Placeholders"
section: `${1:another ${2:placeholder}}`.

Adding `+` quantifier to `any` seems to be more precise.

This seems to also fix the grammar for initial snippet input (which is
implied to be `any`).
@echasnovski echasnovski force-pushed the snippet_placeholder-any branch from 96ca492 to ceb1ada Compare December 6, 2024 11:50
@echasnovski
Copy link
Author

I finally remembered why I chose the route of not updating any directly: it is a better fit for resolving #2033. Its one solution (shown in the patch) requires any and placeholders be separate entities. But I agree that as an isolated change modifying any directly should be better, so I pushed it.

@dbaeumer, maybe you have own reservations about this (and #2033, for that matter)?

@ribru17
Copy link
Contributor

ribru17 commented Dec 6, 2024

Ah makes sense, thx for linking

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.

2 participants