Remove implicit g flag from snippet transformations

[savetheclocktower]

Breaks backward-compatibility, but fixes a bug because this was never supposed to work this way.

Description of the Change

I added the original support for snippet transformations back in the day. It took me more than a year to realize that I’d implemented it wrong, and that the regex syntax for transformations did not have an implied “global” flag.

As a result, transformations have behaved differently (in a subtle way) in Atom/Pulsar than in TextMate, VSCode, and probably any other editor that supports snippets. Since snippet syntax is now part of the LSP specification, this needs fixing, even if it will break backward-compatibility.

Alternate Designs

None were considered. This fix was straightforward and touched two lines of the PEG file.

Benefits

Some snippets that worked fine in other IDEs would’ve behaved differently in Pulsar, but would now behave identically after this change.

Possible Drawbacks

Breaking backward-compatibility. But here’s why I don’t think this will affect many people:

• It’s an advanced feature that I think most users have never used, even in other IDEs.
• Though the snippet transformation feature was present in TextMate and other IDEs, it was never documented in the Flight Manual, so I don’t think it ever saw widespread usage in Atom.
• In the ~17 months it took me to realize that this was broken, zero people opened an issue in atom/snippets to complain.

And even if all that weren’t true, it’s a bullet that we’d need to bite anyway, because it does us no good to hang onto a subtle incompatibility in snippet syntax.

Applicable Issues

atom/snippets#288 contains a lot of further work by @Aerijo that is worth revisiting. At the time, it didn’t get much traction because it was a massive rewrite, and @Aerijo eventually decided to write their own package to replace snippets, à la autocomplete-plus. Attaining full feature parity with LSP snippets is still a good goal, and I think we can get there incrementally.

[confused-Techie]

Gotta say we appreciate the contribution!

Amazing to have someone that had originally implemented this behavior helping us out with something we obviously didn't know was an issue!

Especially the fact you've ensured tests are happy, and that you've added relevant tests is a huge plus!

Thanks again for this and we will have to take a look to review this further

[savetheclocktower]

Can I get eyes on this so I can open the PR for issue #9? I know this is a bit esoteric, so shout at me on Discord if you have questions or just want to verify that this matches VSCode's and TextMate's behavior.

[confused-Techie]

Alright, so I've taken another review of this, and read through all the awesome links provided.

But I'm gonna leave an approve here. While I might not be the most familiar with this code base there's nothing new to understand on the tests nor the changes you made, really just a lack of thorough understanding on how this will effect things on a larger scale.

But like you've mentioned this was a feature that had zero documentation and may not have the most widespread usage, and this simple change gets things to work how anyone coming to the platform would expect it to, while also getting us closer to meeting the expected specification on this feature.

So with those goals in mind, and solid looking code, with responsibly written tests, I fully support this being merged and think we should go ahead and do so.

With that said I'll give a ping to the relevant users over in @pulsar-edit/core & @pulsar-edit/packages to see if anyone else has any thoughts on this.

As this has already been up for some time if we still get no activity in the next week or so I'll be more than happy to go ahead and merge this PR and get a new tagged version on snippets out.

---

Also feel I should say that out of the teams I've pinged I have a feeling that you, @savetheclocktower might actually be the one most knowledgeable within the org with this package, and specifically this feature because rather obviously you originally created it, so trusting your judgement here.

Otherwise again, thanks for all of your hard work within Pulsar recently, it's seriously appreciated even if we don't always have the time to get to everything

---

PS: Quick edit note, the tests here are inaccurate. And should be updated to our new test runner as soon as possible. Out of scope for this PR but for other reviewers the tests here can't be trusted.

[savetheclocktower]

Yeah, the CI failures are addressed in #10. If #10 is landed first, I can rebase this PR so that it gets those fixes.

[Spiker985]

@savetheclocktower Just merged #10, feel free to rebase

[savetheclocktower]

All green! This is ready to land at someone's convenience.