search

pulsar-edit / ppm

Pulsar Package Manager
MIT License
35 stars 13 forks source link

Fix bugs found in `publish` after 1.112 release #116

Closed savetheclocktower closed 8 months ago

savetheclocktower commented 8 months ago

These are just the ones I found when trying to run ppm publish patch, but I'm sure there are other bugs. The PPM rewrite was gigantic.

savetheclocktower commented 8 months ago

Added some tests. I don't love them — they construct a false reality! — but they're better than the nothing that we had.

asiloisad commented 8 months ago

I probably caught the same bug

image

savetheclocktower commented 8 months ago

These new tests pass locally for me. I'll get around to fixing them.

savetheclocktower commented 8 months ago

OK, found the problem with the tests. CI is good. This one is definitely a candidate for “squash and merge.”

Daeraxa commented 8 months ago

Do we need a 1.112.1 for this?

savetheclocktower commented 8 months ago

Yes. We should test other aspects of ppm to see if other bugfixes can be addressed in a patch release, but so far the only bugs reported are with publishing.

DeeDeeG commented 8 months ago

Okay, I saved a copy of the original branch at https://github.com/DeeDeeG/ppm/tree/bug-fixes-original, for reference. (Tip of said branch being https://github.com/pulsar-edit/ppm/commit/dfd19911f56623d7d8d83ccee54b5a59f3febbf0).

Branch protection rules won't let me push master branch without it coming from a PR, so I'll update (force push) this PR with the tidied up commit history real quick...

DeeDeeG commented 8 months ago

Oh, actually... Hmm. I think I accidentally bypassed the branch protection rules (that I didn't recall we had set up) with admin powers.

DeeDeeG commented 8 months ago

Manually merged via commit https://github.com/pulsar-edit/ppm/commit/f660a7a70d9ce0b7b5f7c0fe1e2af6890af205fa (a merge commit that I prepared on the CLI instead, using the usual text that would have been used if it were prepared via the github.com UI).

DeeDeeG commented 8 months ago

So, if you prefer the PR to have a purple icon and say "merged" I'll back out the manual merge and do it via the GUI here, but otherwise this'll be red and "closed", but the changes are in master branch at the moment one way or the other.

confused-Techie commented 8 months ago

@DeeDeeG For tracking and changelog reasons I'd say lets properly merge this one, but at the end of the day, as long as we remember the changes are in here, I suppose it doesn't matter too much

DeeDeeG commented 8 months ago

CI passed with my rebased commits (as they should, I didn't change any code at all), and I can confirm the "diff" between the original and the rebased version is none, only thing I did was the cleaner commit history.

Finally merging (via the github.com PR UI this time). Thanks much for the fix and added tests!