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

Raise Error: Add Faraday::RequestTimeoutError #1513

Merged
merged 3 commits into from Jun 29, 2023

Conversation

tisba
Copy link
Contributor

@tisba tisba commented Jun 29, 2023

Fixes #1511

This PR adds Faraday::RequestTimeoutError and raises it on HTTP 408 responses when the Raise Error Middleware is used.

I also took the liberty to hint out how to get eventmachine installed with Ruby >= 3.0 to work on docs/.

Let me know if this works for you or if you expect something in addition. I basically just copy-pasted the new error looking at the surrounding code 😀

Copy link
Member

@olleolleolle olleolleolle left a comment

Choose a reason for hiding this comment

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

Thank you, @tisba!

The MDN URLs look great. The new error class follows convention.

Let's get this merged!

@olleolleolle olleolleolle merged commit 83676df into lostisland:main Jun 29, 2023
8 checks passed
Copy link
Member

@iMacTia iMacTia left a comment

Choose a reason for hiding this comment

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

Thank you for the addition @tisba, and love the MDN links as well ❤️
I left a small comment on the docs addition for eventmachine as I believe those should not be in this repo, but moved to the relevant ones.

Comment on lines +21 to +28
On newer Ruby versions (>= 3.0) `eventmachine` needs a little help to find OpenSSL 1.1 to get compiled (see <https://github.com/eventmachine/eventmachine/issues/936>). If you're using homebrew on macOS, you can do this:

```bash
brew install openssl@1.1
bundle config build.eventmachine --with-openssl-dir=$(brew --prefix openssl@1.1)
bundle install
```

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for pointing this out @tisba.
Faraday doesn't bundle an eventmachine adapter since v2.0, so people should not have this issue unless they're using https://github.com/lostisland/faraday-em_http or https://github.com/lostisland/faraday-em_synchrony.

I'd suggest moving this suggestion in those gems because eventmachine is not a dependency in this gem

Copy link
Member

Choose a reason for hiding this comment

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

I thought this was about the "get Jekyll running, to view docs"? I could be wrong!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What @olleolleolle said. This is about getting the docs running locally.

Copy link
Member

Choose a reason for hiding this comment

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

really?! Well that's a surprise 😄!
In that case, thanks for the addition 🙏 !

Copy link
Member

Choose a reason for hiding this comment

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

In Gemfile:
  github-pages was resolved to 228, which depends on
    jekyll-avatar was resolved to 0.7.0, which depends on
      jekyll was resolved to 3.9.3, which depends on
        em-websocket was resolved to 0.5.3, which depends on
          eventmachine

yup, coming form the docs/Gemfile, good catch!

@tisba tisba deleted the add-request-timeout-client-error branch June 29, 2023 15:10
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.

Missing HTTP 408 Request Timeout specific Faraday ClientError?
3 participants