Do not warn even if faraday-retry gem is missing #1706
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 |
| begin | ||
| require 'faraday/retry' | ||
| rescue LoadError | ||
| Octokit::Warnable.octokit_warn 'To use retry middleware with Faraday v2.0+, install `faraday-retry` gem' |
There was a problem hiding this comment.
This warning is the only thing that would even let you know that this gem does retries. There is nothing in the README about this. This would be better as an explicit option to enable retries.
There was a problem hiding this comment.
So do you suggest disabling retries by default?
There was a problem hiding this comment.
That would be the best way to remove the warning. The retry functionality needs to be better documented.
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.
| 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
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!