Do not warn even if faraday-retry gem is missing#1706
Do not warn even if faraday-retry gem is missing#1706akihikodaki wants to merge 4 commits intooctokit:mainfrom
Conversation
|
👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labeled with |
| Next, insert `Octokit::Middleware::Retry` before `Octokit::Response::RaiseError`: | ||
|
|
||
| ```ruby | ||
| Octokit.middleware.insert Octokit::Response::RaiseError, Octokit::Middleware::Retry |
There was a problem hiding this comment.
I would think configuring it on the client would be better
Octokit::Client.new(auto_retry: true)and then in the connection sawyer options you can insert the retry middleware if @auto_retry
conn_opts[:builder].insert(0, *retry_middleware) if @auto_retryThere was a problem hiding this comment.
While having a parameter for Octokit::Client.new is more concise, there are a few advantages of explicitly requiring to interact with middleware:
- A user can specify additional options.
- It is clear that it interacts with middleware.
- It is consistent with the suggestion for caching, which follows this section in
README.md.
So there is some trade-off. What do you prefer?
There was a problem hiding this comment.
What happens if you want one client that retries and another that doesn't. Setting on Octokit.middleware is global.
You probably also want a maintainer to chime in here...
There was a problem hiding this comment.
You can also configure an individual instance; see: https://github.com/octokit/octokit.rb/pull/1706/files#diff-7ee18df1929eee9ac41b3ffa5f60cc4904aeb944ff7be6034721b1a76dc2798dR520
|
👋 Hey Friends, this pull request has been automatically marked as |
It makes little sense to emit a warning saying "to use retry middleware" when the user does not request that. Remove the warning.
Duplicate the default middleware stack so that it won't be modified.
Octokit::Middleware::Retry allows incorporating the retry logic without relying on the default middleware stack.
No retry middleware is included in Faraday 2.x and retrying requires Faraday Retry gem. The default middlware stack used to include a retry middleware only when available, which means the behavior of Octokit implicitly changes depending on to the Faraday version or the presence of Faraday Retry gem. Remove the retry middleware from the default middleware stack to avoid such an implicit behavioral change.
|
Rebased to v10.0.0. |
2 participants
It makes little sense to emit a warning saying "to use retry middleware" when the user does not request that. Remove the warning.
Resolves #1567
Before the change?
After the change?
Pull request checklist
Does this introduce a breaking change?
Please see our docs on breaking changes to help!