Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Overhaul snippet syntax for feature support #308

Closed
wants to merge 15 commits into from

Conversation

Aerijo
Copy link
Contributor

@Aerijo Aerijo commented Jul 16, 2020

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions

Description of the Change

Rewrite of the snippet parser to make way for adding new features. In particular:

  • Adds variable parsing (old behaviour was to discard them in the parser)
  • Adds choice parsing
  • Escapes only work when the character may be interpreted as non-plain text otherwise (old behaviour allows anything to be escaped)
    • How escapes should work are fuzzy in the LSP spec, but VS Code goes with the minimal escaping version so I'd prefer to match it to maximise compatibility.
  • Transformations no longer use a : before the regex (old behaviour used one)
  • Transformation regex no longer defaults with a global flag. This allows matching that was impossible before, such as only the first occurrence of a pattern.
  • Overhauled format nodes. They now support the ${n} syntax, as well as conditional expansion into arbitrary replace items (see diagram below). I felt this was better than only allowing plain text, and doesn't have the complexity of nesting transformations / tab stops because the nested format backreferences still only refer to the original capture groups.
  • Added a lot of tests for all the syntax. I tried to add a test to justify every part of the grammar (i.e., removing an alternative somewhere would cause a test to fail).

Most of the above is only in the syntax, there is no support for actually using them yet.

These changes are controlled via a config setting. It is defaulted to the new syntax, as the idea is to eventually drop the old if possible, but it allows users time to adjust their snippets if they rely on the old behaviour. I can write a conversion guide (or maybe even automatically translate them), but it's old and unmaintained packages that will be difficult to fix. The tests all pass for both versions, except for the syntax tests and ones that had to be corrected for regex no longer being global.

The following is a diagram of what snippets can be made out of. I don't know how useful it is for just changing the syntax, but it was useful to me when writing the tests.
SnippetsContainment
Note Escape refers to an inline escape modifier like \u, and Modifier refers to a modifier such as ${1:/upcase}.

I've also renamed some things (substitution -> transformation, escape -> modifier) to closer match their function. This can be reverted if unwanted.

Alternate Designs

The permissive escaping rules could kept, but then snippets could slightly break across editors. This is would be annoying for language servers, as they are meant to support snippet syntax defined in the LSP spec, which is based on the VS Code implementation. Slight differences across editors could require a lot of special casing.

Something to consider is "interpolated shell syntax". TextMate has this, where content in (something like) `content` is evaluated in the shell ( imagine we'd eval it in Atom). I don't know if this is something we want to do, it seems like a security risk (we have no control over where snippets come from). But now would be the best time to add syntax support for it, even if it doesn't get implemented.

I originally meant to support switching between the syntaxes without restarting (the setup for that is in this PR), but don't want to work more on that until snippet evaluation is settled (#306).

Benefits

Lays the foundation for more powerful snippets

Possible Drawbacks

It's a breaking change. At the very least the Atom core packages need to be checked for compatibility, and if there's some way to check against community packages that would be good too. I'm most concerned about LaTeX snippets because of the escape rules changing, but I have access to the main LaTeX packages so can help out there.

But after this, changes should only be to implement the functionality so wouldn't be breaking (unless users have some weird reliance on the unimplemented behaviour).

Applicable Issues

Progress for #288

@Aerijo
Copy link
Contributor Author

Aerijo commented Jul 31, 2020

Finding and fixing anything broken by this change is more effort than I want to put into at this time. Instead I've focused on reimplementing everything from the ground up. Once (if) it matures, I may revisit making changes to this package, but for now I'm focusing all my snippet efforts on the new package.

https://github.com/Aerijo/snippets-plus

@Aerijo Aerijo closed this Jul 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant