Asyncify without topmost interface #95

2colours · 2023-09-16T00:47:00Z

The second attempt, following #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 · 2023-09-16T00:55:13Z

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 · 2023-09-23T01:06:00Z

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:

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.
set static promiseBased = true for the class
go through the methods, checking their implementation to pick a promisification strategy
check the callers of the methods and rewrite those calls appropriately
as part of this process, rewrite the run method as well, check the tests, fix the mistakes...

2colours · 2023-09-29T11:22:07Z

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

some functions need to be mocked differently - this is pretty obvious
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 · 2023-09-30T12:51:31Z

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 · 2023-10-01T16:03:58Z

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.

No functional changes so far.

No tests broke. This is a functional change but very much a simple and basic transformation.

All tests succeed. This is a wrapper implementation, like for printVersions.

Under similar terms as the previous ones. Tests passing.

There are failing tests that I isolated in a spec2 folder. - develop-spec seems like it "mocks out" the resolution of the Promise, causing some tests to fail - stars-spec explodes with a "trying to reopen HTTP port" error, probably some cleanup section couldn't run - test-spec mocks I/O of processes and for some reason, the tests won't produce the right output - unpublish-spec fails at the unpublishing itself, there is a lot of mocking but the failure looks too legit nevertheless - upgrade-spec first times out and then dies with the "HTTP port" error, like stars-spec

It seems that install-spec was the one that didn't close the server, causing the remaining tests using a server to break.

And the polishment of the corresponding test which passes now. 🎉 getRepositoryUrl has a true resolve-reject interface, the rest always resolves, mostly wrapped by the Promise constructor

All the previous tests pass still. Finally, a function that could be changed to an async flow, rather than just wrapped into a Promise. The calls have been modified appropriately.

Tests still pass. It's a really thin Promise wrapper on the existing code.

Tiny file, tiny wrapper, tests pass.

Tests pass, tiny wrapper of tiny command

A bit fatter wrapping for a bit fatter command. Tests are passing.

Tests passing, trivial wrapper used.

Tests passing. Trivial wrapper added.

Tests passing. Two of the three methods could immediately be turned into async 🎉

It was so small that it was easier to do than not do. Only install.js was modified which is currently not tested.

Tests still passing. The main change is that printVersions in apm-cli could be promisified as a result.

Tests passing. run could be asyncified right away. I still want to tackle generateFromTemplate which is ugly.

Tests still passing.

Tests still passing. It was so banal that it could be asyncified right away. As a positive side effect, linkPackage in develop.js doesn't have to be wrapped with the Promise constructor anymore.

This command is broken on master as well but install.js calls the rebuild method so I went ahead and asyncified it. It still doesn't work (I think the problem is related to asar and faulty resolution of "resource paths") but the tests are still passing.

This is a cosmetic phase of the iterative development. I'm eliminating as much of callbacks as possible to see what work is left. All tests passing.

Similar to the last commit. All tests passing.

Similar to the previous two commits. All tests passing. It seems to me that probably only request.js and login.js are still the old way...

Similar to the previous cosmetic changes. All tests still passing.

Similar to the previous ones. All tests passing.

*without the main user-facing interface because that's deliberately not a Node-style callback. All tests are passing.

This is the main reason I didn't want to touch Login before. It wasn't as scary as it first seemed. All tests passing, the only thing left is the Login command itself.

All tests passing - but tests are too lenient because they don't actually test the prompt path... That seems to work, too. The interface is still unmodified because this was the only command that had separate error and return value.

Tests (and manual check) passes. Now the command has no special return value, and getTokenOrLogin calls a method that can have one.

The recent PR pulsar-edit#100 made me want to check this command and I noticed I messed up at two places to make this work. Rather obvious fixes for a fortunate catch...

Now that everything is Promise-based (and anything new should also be), it's pointless to keep an option for classes to avoid wrapping into Promises. Nothing should be wrapped anymore.

All tests still passing. Some unused code and leftover wrapping removed.

DeeDeeG · 2023-10-01T18:52:14Z

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: 9dd95c6
New commit after I rebased: 0e865d2

(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.

confused-Techie

Just wanna say this is some fantastic work! Seriously, huge undertaking and I'm very much impressed.

I've left a few comments around about improvements or questions, or even just some comments. So please address them when you've got the chance, but besides those few items overall this looks perfect, and as for the code side of this review, I'd say it's basically good to go!

With that I'll add I'm relying on the manual testing that @DeeDeeG has done, and the automated tests to point out any unknown flaws in the logic, but as for the code it looks identical and completely valid, so I think we are good to go there!

src/apm-cli.js

src/apm.js

src/auth.js

src/apm.js

src/ci.js

src/enable.js

src/fs.js

src/git.js

src/link.js

src/install.js

Co-authored-by: confused_techie <dev@lhbasics.com>

Spiker985 · 2023-11-08T22:24:29Z

@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

confused-Techie

Alright, I'm gonna add an approval on here, but do want to make a clarification.

I've reviewed only the code side of changes, ensuring accuracy, and coherence to how things previously functioned. Ensuring that the only essential changes were made, albeit with some tiny stylistic and quality of life improvements.

So the code itself looks good, my concerns have been addressed, and otherwise this PR looks incredibly solid.

But as for functionality, I'm trusting two things, one @DeeDeeG themselves having previously tested these changes, the author of this PR themselves also testing these changes, and lastly the fact that our specs are happy, and report running the exact same amount of tests as the most recent PRs here, indicating that not much has changed coverage wise (Hopefully).

With that said, I'll add my approval here, but would be more than happy to hear from @DeeDeeG about any of their concerns coverage or functionality wise, if they exist.

Otherwise I fully support us getting this in, and getting it updated in Pulsar itself

DeeDeeG

I tested a bunch of stuff before, and the PR is frozen from any meaningful changes since then, so it still stands.

I may not have gotten to every subcommand, but I'm comfortable enough getting this into Rolling hopefully with a few days lead time before Regular. (Tangent: That timing would be kind of tight, and I don't want to rush people about it, to be honest. But this is a tad off-topic to this PR itself.)

If we would have to do a hotfix release of Pulsar to roll back ppm and do a "try two" of this PR (even if maintainers have to handle that) that is all doable.

So, at this point I can "lite approve" (meaning, despite best intentions, I didn't have time to review the code myself, so I am approving on a basis of being willing to troubleshoot and manage any issues post-merge.) Let's go for it, I think.

DeeDeeG · 2023-11-12T06:59:14Z

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!

confused-Techie mentioned this pull request Sep 22, 2023

Convert to Async #88

Closed

2colours force-pushed the asyncify-without-top branch from c8072c9 to 8d33f87 Compare September 22, 2023 23:50

2colours force-pushed the asyncify-without-top branch from e473f5d to de392dd Compare September 29, 2023 18:51

DeeDeeG mentioned this pull request Oct 1, 2023

src: Rebrand two lines of "ppm --version" output #100

Merged

2colours marked this pull request as ready for review October 1, 2023 15:58

Polgár Márton added 20 commits October 1, 2023 14:49

Beautification of apm-cli

8b03d4c

No functional changes so far.

Wrap printVersions

8095925

No tests broke. This is a functional change but very much a simple and basic transformation.

Promisify getPythonVersion

aa6b2e9

All tests succeed. This is a wrapper implementation, like for printVersions.

Promisify getPythonVersion

debca29

Under similar terms as the previous ones. Tests passing.

Better sort broken testfiles

fe3bf87

It seems that install-spec was the one that didn't close the server, causing the remaining tests using a server to break.

Cleanup and promisification of the develop command

b092ca5

And the polishment of the corresponding test which passes now. 🎉 getRepositoryUrl has a true resolve-reject interface, the rest always resolves, mostly wrapped by the Promise constructor

auth.js getToken asyncified

1baaa60

All the previous tests pass still. Finally, a function that could be changed to an async flow, rather than just wrapped into a Promise. The calls have been modified appropriately.

Promisify ci.js

cf85347

Tests still pass. It's a really thin Promise wrapper on the existing code.

Promisification of clean.js

ccb5d3e

Tiny file, tiny wrapper, tests pass.

Promisification of config.js

34d78f7

Tests pass, tiny wrapper of tiny command

Promisification of disable.js

689e8e6

A bit fatter wrapping for a bit fatter command. Tests are passing.

Promisification of docs.js

2e25bb0

Tests passing, trivial wrapper used.

Promisification of enable.js

11b6f14

Tests passing. Trivial wrapper added.

Promisification of featured.js

9823c21

Tests passing. Two of the three methods could immediately be turned into async 🎉

Promisification of fs.js

a4e6d70

It was so small that it was easier to do than not do. Only install.js was modified which is currently not tested.

Promisify git.js

1093f4a

Tests still passing. The main change is that printVersions in apm-cli could be promisified as a result.

Promisification of init.js pt.1

df0adba

Tests passing. run could be asyncified right away. I still want to tackle generateFromTemplate which is ugly.

init.js pt2: make generateFromTemplate look a bit more sensible

47f7dba

Tests still passing.

Asyncification of link.js

6da61f5

Tests still passing. It was so banal that it could be asyncified right away. As a positive side effect, linkPackage in develop.js doesn't have to be wrapped with the Promise constructor anymore.

Polgár Márton added 13 commits October 1, 2023 14:49

Eliminating all callbacks from ci.js

0aaac88

This is a cosmetic phase of the iterative development. I'm eliminating as much of callbacks as possible to see what work is left. All tests passing.

Eliminating all callbacks from develop.js

cfac2bf

Similar to the last commit. All tests passing.

Eliminating all callbacks from star.js

0a3e5c2

Similar to the previous two commits. All tests passing. It seems to me that probably only request.js and login.js are still the old way...

Eliminating all callbacks from unstar.js

8ba5e83

Similar to the previous cosmetic changes. All tests still passing.

Eliminating all callbacks from upgrade.js

316d9fe

Similar to the previous ones. All tests passing.

Promisification of request.js

ae90d47

*without the main user-facing interface because that's deliberately not a Node-style callback. All tests are passing.

Asyncification of getTokenOrLogin

8576053

This is the main reason I didn't want to touch Login before. It wasn't as scary as it first seemed. All tests passing, the only thing left is the Login command itself.

Fully rework login.js into async

ba12df8

Tests (and manual check) passes. Now the command has no special return value, and getTokenOrLogin calls a method that can have one.

Fix --version flag

71278ea

The recent PR pulsar-edit#100 made me want to check this command and I noticed I messed up at two places to make this work. Rather obvious fixes for a fortunate catch...

Eliminating the promiseBased setting of commands

2ba286b

Now that everything is Promise-based (and anything new should also be), it's pointless to keep an option for classes to avoid wrapping into Promises. Nothing should be wrapped anymore.

Eliminating unnecessary Promise contructors in favor of async

0e865d2

All tests still passing. Some unused code and leftover wrapping removed.

DeeDeeG force-pushed the asyncify-without-top branch from 9dd95c6 to 0e865d2 Compare October 1, 2023 18:52

confused-Techie requested changes Oct 18, 2023

View reviewed changes

2colours and others added 3 commits October 18, 2023 04:03

s/Atom/Pulsar/ on the go

72f929d

Co-authored-by: confused_techie <dev@lhbasics.com>

s/Atom/Pulsar/ in install

e35010a

Co-authored-by: confused_techie <dev@lhbasics.com>

s/Atom/Pulsar/ in enable

4d0b679

Co-authored-by: confused_techie <dev@lhbasics.com>

DeeDeeG mentioned this pull request Oct 23, 2023

CI: Work around a weird bug in Yarn v1.x #101

Merged

confused-Techie approved these changes Nov 9, 2023

View reviewed changes

DeeDeeG reviewed Nov 11, 2023

View reviewed changes

DeeDeeG merged commit 13fb284 into pulsar-edit:master Nov 12, 2023
8 of 11 checks passed

2colours deleted the asyncify-without-top branch November 12, 2023 09:19

2colours mentioned this pull request Nov 16, 2023

Atom->Pulsar-Edit Raku/atom-language#100

Open

4 tasks

DeeDeeG mentioned this pull request Nov 16, 2023

ppm: Update ppm to commit 13fb2845e00d7e04c2461f93 pulsar-edit/pulsar#809

Merged

DeeDeeG mentioned this pull request Dec 13, 2023

src: Delete unused code in uninstall.js #104

Merged

DeeDeeG mentioned this pull request Apr 20, 2024

Add ability for PPM to run electron-rebuild automatically #126

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Asyncify without topmost interface #95

Asyncify without topmost interface #95

2colours commented Sep 16, 2023 •

edited

Loading

2colours commented Sep 16, 2023

2colours commented Sep 23, 2023

2colours commented Sep 29, 2023

2colours commented Sep 30, 2023

2colours commented Oct 1, 2023

DeeDeeG commented Oct 1, 2023 •

edited

Loading

confused-Techie left a comment

Spiker985 commented Nov 8, 2023

confused-Techie left a comment

DeeDeeG left a comment

DeeDeeG commented Nov 12, 2023 •

edited

Loading

Asyncify without topmost interface #95

Asyncify without topmost interface #95

Conversation

2colours commented Sep 16, 2023 • edited Loading

2colours commented Sep 16, 2023

2colours commented Sep 23, 2023

2colours commented Sep 29, 2023

2colours commented Sep 30, 2023

2colours commented Oct 1, 2023

DeeDeeG commented Oct 1, 2023 • edited Loading

confused-Techie left a comment

Choose a reason for hiding this comment

Spiker985 commented Nov 8, 2023

confused-Techie left a comment

Choose a reason for hiding this comment

DeeDeeG left a comment

Choose a reason for hiding this comment

DeeDeeG commented Nov 12, 2023 • edited Loading

2colours commented Sep 16, 2023 •

edited

Loading

DeeDeeG commented Oct 1, 2023 •

edited

Loading

DeeDeeG commented Nov 12, 2023 •

edited

Loading