search

pulsar-edit / ppm

Pulsar Package Manager
MIT License
35 stars 13 forks source link

Decaffeinate clean spec #66

Closed 2colours closed 1 year ago

2colours commented 1 year ago

This is meant to supersede https://github.com/pulsar-edit/ppm/pull/22. Like with https://github.com/pulsar-edit/ppm/pull/65, I was trying to stay away from require-ing the config file while incorporating the configurable node version commits. Also, eliminated some closure variables with their initialization.

2colours commented 1 year ago

I don't totally understand the aversion to requiring a JSON file, but I suppose the change isn't worth worrying about either. EDIT: It would be too much hassle to start worrying about this in this PR when the approach was already accepted in #65, so disregard this comment, I guess.

Anyway... I would hope that I said my biggest arguments under that PR - of course, these are still rather minor things but then the change itself is rather minor, too. It's a bit bothering that the code needs to rely on CommonJS either way. :(

There is a redundant commit deleting spec/spec-helper.coffee even though it is also deleted on the target decaf branch, this will add a little noise to the commit log but beyond that it is a non-issue, just something I saw and will point out as I'm reviewing. Again, I don't think this needs to change -- I would be fine merging this as-is. If in the future you rebase commits for any decaf PRs, you can leave out the commit deleting spec/spec-helper.coffee for cleanest/simplest commit history. But not a huge deal.

Yep... honestly, I never quite understood merge. I understand rebase and I thought the outcome would essentially be the same but apparently not, yuck. I just wanted to avoid PRs building on top of each other. I don't know if you can rebase these branches but even at the end, I suppose an interactive rebase can help clear up unwanted commits, in case.