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

Bump snippets dependency to 1.8.0 #972

Merged

Conversation

savetheclocktower
Copy link
Contributor

This bump incorporates the two features that just landed in snippets:

  • Ability to use “simple” transformation flags like /upcase and /camelCase within sed-style snippet transformation replacements
  • Support for the snippet variables LINE_COMMENT, BLOCK_COMMENT_START, and BLOCK_COMMENT_END

After landing those, I tagged version 1.8.0 of snippets and pushed it to the repo. This PR updates Pulsar to use the new version.

There's a future task to bring snippets into the core Pulsar repo. I don't have much of an opinion on when that should happen. It would be nice, though, if I could convince more of the core team that snippets were good and worth using, so that more of you had opinions on snippets and were able to understand these changes more deeply. :-)

Since CI is green on snippets and should end up green on this PR, I think this is an easy merge, but I'm happy to get feedback on this one. Checking out this PR and running Pulsar from source will allow people to do manual testing of these new snippets features if they like.

Copy link
Member

@confused-Techie confused-Techie left a comment

Choose a reason for hiding this comment

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

I'm happy to trust the green CI on this one as a simple dependency bump on this end, if we want to merge in time for the regular release

@savetheclocktower savetheclocktower merged commit 35dd1cc into pulsar-edit:master Apr 14, 2024
104 checks passed
@savetheclocktower savetheclocktower deleted the bump-snippets-to-1-8-0 branch April 14, 2024 18:03
@DeeDeeG
Copy link
Member

DeeDeeG commented Apr 15, 2024

FWIW, snippets is already a bundled/core package, and as long as we run its tests in CI I don't think there's a super important reason to keep it its own repo.

One reason I like github package being separate is just that it's so huge and heavy, and that I'd like to be able to prototype fixes to its CI separately. (Not clear to me whether running its specs here or in a separate repo is any easier, help wanted to be honest.) But yeah, I son't personally think we especially need snippets to stay a separate repo, given it's already bundled.

Oh, and I suppose being able to do ppm install https://github.com/pulsar-edit/github is a nice thing. Dunno how often we really need to be doing that for snippets, though.

@savetheclocktower
Copy link
Contributor Author

But yeah, I son't personally think we especially need snippets to stay a separate repo, given it's already bundled.

I think it was only left external because the PR that added variables hadn't yet been reviewed and I was too busy at the time to push for its inclusion. But that PR landed long ago.

Oh, and I suppose being able to do ppm install https://github.com/pulsar-edit/github is a nice thing. Dunno how often we really need to be doing that for snippets, though.

Even if you don't want to run Pulsar entirely from scratch, you can always cd into one of the directories in packages and then do ppm link . to substitute one builtin package's code for the version that's bundled with Pulsar. (Another handy thing that I hope to document better so that contributors don't feel like they need the entire Electron toolchain just to contribute fixes.)

@confused-Techie
Copy link
Member

Yeah according to #512 it says snippets is still excluded because Per core maintainers requests, this item will not be bundled until later notice which I'd assume was you when that PR was still open. But if we are good to merge it I'll be more than happy to do that soon.

Otherwise I'd love to get github bundled here as well, but suppose that's still slightly up in the air reading this discussion

@DeeDeeG
Copy link
Member

DeeDeeG commented Apr 16, 2024

I'm unsure if I have rational reasons left for not bundling github package.

I do worry that at the moment it will mean we simply run no tests for it whatsoever in CI, since there is a quirk of core repo's test runner, where because the github package's test dir is test instead of spec, that means our CI doesn't notice there are any tests to run (looking in the wrong dir) and github package basically passes due to "no failing specs" due to "no specs"(!).

On the other hand, specs are so broken at github's own repo that we're not getting the benefit of it separately there, either.

Would be wonderful to have the github package's tests passing again.

@confused-Techie confused-Techie mentioned this pull request Apr 28, 2024
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