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

New feature: RaiseError #792

Merged
merged 3 commits into from
Nov 11, 2024
Merged

New feature: RaiseError #792

merged 3 commits into from
Nov 11, 2024

Conversation

c960657
Copy link
Contributor

@c960657 c960657 commented Oct 4, 2024

The library raises errors if the server does various unexpected things, e.g. TooManyRedirectsError, but it does not raise if the HTTP status code indicates a client error (4xx) or server (5xx) error.

Sometimes you are interested in non-successful responses, and sometimes you just want to treat them as any other error triggered by the server.

This PR adds a new feature that will raise an exception on status codes representing an error. If you want to skip this for specific codes, you can specify them in the ignore option.

Comment on lines 13 to 17
if response.code >= 400 && !@ignore.include?(response.code)
raise HTTP::StatusError, "Unexpected status code #{response.code}"
end

response
Copy link
Member

Choose a reason for hiding this comment

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

WDYT about:

Suggested change
if response.code >= 400 && !@ignore.include?(response.code)
raise HTTP::StatusError, "Unexpected status code #{response.code}"
end
response
return response if response.code < 400
return response if @ignore.include?(response.code)
raise HTTP::StatusError, response

Then changing HTTP::StatusError to:

   class StatusError < ResponseError
     attr_reader :response

     def initialize(response)
       @response = response

       super("Unexpected status code #{response.code}")
     end
   end

let(:ignored) { [500] }

it "raises" do
expect { result }.to raise_error(HTTP::StatusError, "Unexpected status code 500")
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't it not raise an error if 500 is ignored?


context "when error status is ignored" do
let(:status) { 500 }
let(:ignored) { [500] }
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let(:ignored) { [500] }
let(:ignore) { [500] }

@c960657
Copy link
Contributor Author

c960657 commented Nov 11, 2024

Thanks for the review. I have addressed your comments.

@c960657 c960657 requested a review from ixti November 11, 2024 21:55
@ixti ixti merged commit db6863a into httprb:main Nov 11, 2024
6 checks passed
@ixti
Copy link
Member

ixti commented Nov 11, 2024

Thank you!

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