Closed DeeDeeG closed 11 months ago
I also considered renaming the function getAtomVersion()
to getPulsarVersion()
and the var atomVersion
to pulsarVersion
, since these are used only internally in src/apm-cli.js
, and they're not exported.
I dunno, since there's so much "atom" this and "apm" that left in the code, I went for the more minimal change, but mighty tempted to rename the var and function names to be accurate to Pulsar, not the old un-rebranded "Atom" var/function names.
Not that it strictly matters, just maybe a bit less confusing to read to have to have a mental model that "Oh, yeah, then they say Atom they mean Pulsar, right..." Hmm.
EDIT: Likewise for the apmVersion
var.
My request is the same as during the decaf process: please, don't make too many changes during the code redesign - or even better: if you do, please make the appropriate modification on the async branch as well. The PR should be writeable by the people who can merge.
@2colours if you want these changes cherry-picked to your branch 2colours:asyncify-without-top
, the one used in PR https://github.com/pulsar-edit/ppm/pull/95, I can do that.
Or if you don't mind, I can merge master
branch into that PR branch to sync them.
I think adding new params to the JSON output and leaving the old ones is the safest move, so I'm intending to go ahead and do that soon. I may real-world test it in production just to see what the impact is, however.
@DeeDeeG right now I'm already very much overdue with other stuff that I procrastinated - this ppm stuff was really useful for that purpose... - so I haven't actually checked the cherry-pick for this one. However, I did rebase when #99 got merged, and that one had to be completely remade, even though it was just a bunch of one-liners. I'm afraid this would be the case here as well; even if it's not a big change, probably it needs to be redone because of the conflicts, in which case trying to merge them one after the other would probably yield merge conflicts once more.
So I'm not sure if it's worth it.
I finalized the PR now, and even though the tests are all passing (actually, a couple more tests as well because I activated the test cases for apm-cli), I would be thankful to get some help with some real-life "integration testing" of it. That's probably more feasible than to try and read essentially the whole code.
If I do a cherry-pick or a merge commit into #95, either way would add this little change just to the end of your branch, so it would be at most one merge conflict and I can manually resolve it correctly since this one is a very simple tiny change.
Yeah, #99 #95 is large, thank you for the context and I think that matter can be addressed in the other PR #99 #95.
The re-enabled spec/apm-cli.js
--> spec/apm-cli-spec.js
tests are passing, and I re-added the old parameters apm:
and atom:
to the JSON output, just to be safe.
Merging on the basis of above reviews and discussion. I know there is new diff, but it seems in line with previous discussion and these changes are very tiny, and I've even tested making the changes in the copy of ppm bundled with Pulsar locally, and it doesn't seem to mind at all, even when I tested with removing the atom:
and apm:
from the JSON output it was fine. But leaving them in just to be conservative.
We're not finding/printing the Atom version, we're finding/printing the Pulsar version. Rebrand this little bit of
ppm --version
.Before:
After: