Asyncify without topmost interface

[2colours]

The second attempt, following https://github.com/pulsar-edit/ppm/pull/93

Long story short: I do think the approach was alright but given the test framework, basically it was a Russian roulette what tests could work, if at all.

This time around, I deliberately omitted the very part I started with last time so that the interface shown towards the executable scripts and the tests themselves would stay callback-based.

[2colours]

So the first couple of functions could be changed no problem, without any test outage.

The big one was the wrapping of the run method of all commands - I isolated the failing tests because there are all sorts of weird interactions.

First, it would be great to fix these tests actually. I'm not sure if that's possible without altering the tests. As a final radical measure, I think these commands could get separate handling behind a wrapper so if all seems lost, maybe there is still a way to keep only these commands callback-based, which I think is still a win, if the basic idea was that the code is ugly. The vast majority of code could still be not-so-ugly, and these commands could rightfully shout "think twice before touching me".

Then, for the other commands, I still think gradually going towards the bottom is a workable paradigm. There can be problems with the mocking but ultimately, if one part is fully Promise-based and eventually asyncified, there will be just genuinely no reason to try to mock certain functions by changing their arguments and such...

[2colours]

I noticed a few things and refined the strategy as well.

• first, the commands that already work with the tests are processed
• then I think the other couple of commands need to be fixed by reducing the tests until a single point of failure remains, solving that, and then gradually adding the tests back if need be
• then lower layers of code can be changed; they will probably gradually allow the higher layers to be refined to a nicer code structure
• the interface of many Promises (what is a value, what is an error) will have to change. I started with a "whenever it's not crystal clear, use the resolve path only" policy. I think I have been overdoing it. Currently, I think only the Promises that actually have to pass resolve or reject on are okay to do that (if at all...). This will have to be redone when I reach back to the higher layers

Anyway, for Commands, the workflow is pretty simple:

1. check if some other command depends on directly calling this Command (with run). If it does, make sure it can be rewritten in a way that satisfies apm-cli and the other command as well, or else it may need to be skipped.
2. set static promiseBased = true for the class
3. go through the methods, checking their implementation to pick a promisification strategy
4. check the callers of the methods and rewrite those calls appropriately
5. as part of this process, rewrite the run method as well, check the tests, fix the mistakes...

[2colours]

I reached a state where all tests succeed with slight alternations:

1. some functions need to be mocked differently - this is pretty obvious
2. the ultimate return value the callback obtains is sometimes downright undefined, not null - I think this is even desirable and there should be less explicit null returns but this is easy to twist in many ways

I would be worried about the install --json path; it's like a whole different command that is carried over to quite low layers of the same functions, and it is the reason why many installation methods need to produce values... I think it needs further testing at the very least; it would be nice to rework it once in the future.

[2colours]

Now, this pull request has "officially" reached the state where (I think) everything that clearly needed an async-based implementation, got one.

The next - and arguably final - stage should consist of:

• "integration testing", i.e actually trying out various functionality, at least with a sample check approach
• refactoring of leftover stuff if we find such
• finalizing the interfaces that this version of code provides, so typically deciding on how the commands work
  • currently, their return value is the error, if there is any
  • and apm-cli did remain callback-based because otherwise all the tests should have been rewritten
  • I don't like these two things, especially the former... but I think we could actually live with them for the time being, and do different PR's to mitigate them

[2colours]

From now on, there are no planned modifications that I still wanted to do, only fixes, or, if it's decided that some interface modification is required in this PR, then that can be added. All in all, I think it's ready for review.

As I said, since the spec is far from exhaustive, some testing is required, others are of course more than welcome to join in with that.

[DeeDeeG]

Rebasing on top of master branch now that #100 is merged, to basically port #100's changes to this PR and sync everything. As was discussed here on GitHub and on the Discord.

Old commit of this branch just before I rebased: 9dd95c6987c7b114e98ddb5a72356f55bb5ca47d New commit after I rebased: 0e865d2e4f43c239fb1ff3e5f79ae88bc9bbb13a

(We can always go back and retrieve the old version if needed for whatever reason. I pushed it to a branch at this repo, for now, just in case: https://github.com/pulsar-edit/ppm/tree/old-asyncify-without-top-pre-rebase)

NOTE: The GitHub UI will show icons for author and committer. It was not showing an author icon before, since the email being used to commit is apparently not configured on @2colours' GitHub account as owned by them. So, GitHub's UI is currently only showing the committer icon (me). But the git metadata is correct that @2colours authored it, and that I re-committed it during my rebase. So, no I didn't author all these commits, and I'm not the author in the metadata, I'm the committer.

[Spiker985]

@DeeDeeG @confused-Techie Since you're more familiar with this PR, is this PR something that you believe we can get in before next release?

Looks like it may want a new review, confused

[DeeDeeG]

Thanks again @2colours for all the hard work on this! I am merging it now.

Hopefully some more async/await style code will make things a lot easier to read and deal with! Thanks!

And P.S. sorry the review took so long, as this was a rather large change, we want to be cautious, but it all looks good from everything I have seen so far! Much appreciated!