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

Asyncify request #146

Closed
wants to merge 2 commits into from
Closed

Conversation

2colours
Copy link
Contributor

@2colours 2colours commented Dec 1, 2024

This is basically #140 but now I didn't mess the branches up.

This is the version that passes all the existing tests.
The HTTP requests used to be handled by a callback that had three arguments: one for the errors if there are any, one for the response and one for the payload within the response.
Now, the error has been turned into rejected promises (exceptions when awaited) and the payload has simply been dropped. It's extracted from the response via the .body attribute.
This is the version that passes all the existing tests.
The HTTP requests used to be handled by a callback that had three arguments: one for the errors if there are any, one for the response and one for the payload within the response.
Now, the error has been turned into rejected promises (exceptions when awaited) and the payload has simply been dropped. It's extracted from the response via the .body attribute.
@confused-Techie
Copy link
Member

Unfortunately, GitHub isn't showing any diff on this PR.

Although the changes you intended in this PR are still available on #147 so I'll see what I can do about reviewing them there, and chime in back here if need be.

@DeeDeeG
Copy link
Member

DeeDeeG commented Dec 15, 2024

Yeah, it looks like there is one commit here with what I presume is the full intended diff, and a second commit with the same message but reverting the changes of the first commit.

It may be possible to review this PR on the basis of the first commit, and to remember to delete the second commit before if/when it would be merged.

@2colours
Copy link
Contributor Author

I think since there has been (seemingly?) plenty of thought going into the other PR which does come with these changes, and not a lot of visible intent to fix/reuse this PR, perhaps it can be just closed at this point. The modifications in #147 still come in distinct commits so I don't think anything gets lost.

@2colours 2colours closed this Dec 22, 2024
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