Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove fs-plus from atom-protocol-handler #170

Merged

Conversation

sertonix
Copy link
Contributor

@sertonix sertonix commented Nov 21, 2022

This replaces the uses of sync with async. It also removes a use of fs-plus

@sertonix sertonix changed the title remove sync atom-protocol-handler async atom-protocol-handler Nov 21, 2022
@confused-Techie
Copy link
Member

This PR looks great, and is absolutely something I think we should be be doing. But I just want to confirm were you personally able to test that things still worked when using this? Such as opening a specific folder or file with the editor still loaded fine?

@sertonix
Copy link
Contributor Author

@confused-Techie To answer your question. I was not able to test this properly.
I also don't know how. Would really appreciate if somebody else could test this.

@confused-Techie
Copy link
Member

So while as you probably guessed, seems nobody got the chance to test this. If you are willing I'd appreciate either opening up this PR to maintainers to push to, or if you could update it from the master branch?

That way we can grab our new test system, and can see at a much quicker glance if this is working as intended. Since I'd love to finally clear our usage of fs-plus

@sertonix
Copy link
Contributor Author

Mantainers should be able to edit this pr.
Also I merged the changes from main.

@confused-Techie confused-Techie changed the title async atom-protocol-handler Remove fs-plus from atom-protocol-handler Jul 21, 2023
@confused-Techie
Copy link
Member

Alright, since this PR has seen nearly zero activity, I've gone ahead and made some changes.

Since this PR originally did two things:

  • Turned this into an async function
  • Removed fs-plus usage

Now, the original author was unsure of how to test things still work while using async handling, I've opted rather than finding a way to test this, to remove the async part of this PR. Since really, I'm willing to assume that turning components of Pulsar into async will require more than just making the function async, where we need to ensure other aspects now know to wait on this happening.

So I've just gone ahead and opted to move forward with removing fs-plus usage from here, and if tests are good then we can go ahead and merge this one, as it's quite outdated at this point

@sertonix
Copy link
Contributor Author

Now that I have looked at the code again I know why I was confident to make the change: It is a callback function.
So more or less the function already was async it just didn't use that.

Copy link
Member

@DeeDeeG DeeDeeG left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to have fixed the editor tests! 👍

If all the other CI tests pass, then LGTM. (Review approve is assuming tests pass.)

@confused-Techie
Copy link
Member

@DeeDeeG Thanks for helping to bring this PR across the finish line!

All tests are passing, and changes look good, as always appreciate your contributions sincerely @sertonix! Lets get this one merged!

@confused-Techie confused-Techie merged commit b39983e into pulsar-edit:master Sep 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants