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

Refactored request error handling #88

Merged
merged 2 commits into from
Jan 29, 2024
Merged

Conversation

dipunm
Copy link
Contributor

@dipunm dipunm commented Sep 19, 2023

Exception is now identifiable as HttpResponseError providing a status code.
Breaking? We no longer look for error messages on ok responses. (HTTP2XX)

Fixes: Not yet.

Description

This is a small PR as part of an ongoing effort to address #86
...

Changes

  • Exception is now identifiable as HttpResponseError providing a status code.
  • Breaking? We no longer look for error messages on ok responses. (HTTP2XX)

PR Tasks

  • Open PR
  • run npm run test --> no failing unit tests
  • run npm run format

@gandlafbtc
Copy link
Collaborator

awesome! looking forward to this

@dipunm
Copy link
Contributor Author

dipunm commented Sep 22, 2023

Should I include the second part into this PR?

I was planning on getting this reviewed and merged and then following up with a second PR.

@gandlafbtc
Copy link
Collaborator

what kind of files would the 2nd pr touch?

@dipunm
Copy link
Contributor Author

dipunm commented Oct 1, 2023 via email

@gandlafbtc
Copy link
Collaborator

okay... i have a PR coming that changes a bunch in CashuWallet.ts. If you can wait a couple days i think we'll have an easier time merging

@gandlafbtc
Copy link
Collaborator

i'll rebase this branch after i merged the branch deterministic-secrets

Exception is now identifiable as HttpResponseError providing a status code.
Breaking? We no longer look for error messages on `ok` responses. (HTTP2XX)
Copy link
Collaborator

@gandlafbtc gandlafbtc left a comment

Choose a reason for hiding this comment

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

Finally rebased onto main after merging other commits!

"Couple days" TM...

I have one comment on the PR! other than that, looks good to me

src/request.ts Show resolved Hide resolved
@gandlafbtc gandlafbtc merged commit 1788185 into cashubtc:main Jan 29, 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.

2 participants