search

pulsar-edit / pulsar

A Community-led Hyper-Hackable Text Editor
https://pulsar-edit.dev
Other
3.33k stars 140 forks source link

Bump `snippets` dependency to 1.8.0 #972

Closed savetheclocktower closed 7 months ago

savetheclocktower commented 7 months ago

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

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.

DeeDeeG commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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.