octokit/octokit.rb

[BUG]: The type of the dependency on faraday-retry is irritating and unclear.

expeehaa opened this issue · 2 comments

What happened?

When using the gem with faraday > 2 without having faraday-retry installed, octokit.rb always prints this warning message.

To use retry middleware with Faraday v2.0+, install `faraday-retry` gem

According to #1486, there are currently 2 ways to disable the warning:

  • disable all warnings by setting OCTOKIT_SILENT=true
  • install faraday-retry

The first option has the side effect of disabling any warning from octokit.rb, which is clearly not my intention. The second option requires me to install a gem, but this appears to actually be kind of optional. I had no issues yet without faraday-retry and usages test if its constants are defined:

if defined?(Faraday::Request::Retry)
retry_exceptions = Faraday::Request::Retry::DEFAULT_EXCEPTIONS + [Octokit::ServerError]
builder.use Faraday::Request::Retry, exceptions: retry_exceptions
elsif defined?(Faraday::Retry::Middleware)
retry_exceptions = Faraday::Retry::Middleware::DEFAULT_EXCEPTIONS + [Octokit::ServerError]
builder.use Faraday::Retry::Middleware, exceptions: retry_exceptions
end

In case I do not want to use faraday-retry for whatever reason, I will therefore always be left with that annoying warning.

From this perspective, it is not clear what kind of dependency octokit.rb actually has on faraday-retry. It is not required, cannot be listed in the gemspec since faraday ~> 1.0 is also supported, but also is not fully optional since the exclusion affects all programs using octokit.rb by printing a warning.

If it is optional, I would expect to at most get a post-install message of the warning (probably rather a README entry), but not a forced warning. If it is required, I would expect require 'octokit.rb' to raise the warning message as an error when require 'faraday/retry' raises a LoadError.

Implementing either of those options seems quite trivial, contrary to what was claimed in #1486 in this comment. Did I miss something there?

I’ld happily create a PR, although I have no idea which options would be preferred, if any.

Versions

octokit.rb = 6.1.1, faraday = 2.7.4 without faraday-retry

Relevant log output

No response

Code of Conduct

  • I agree to follow this project's Code of Conduct

👋 Hey Friends, this issue has been automatically marked as stale because it has no recent activity. It will be closed if no further activity occurs. Please add the Status: Pinned label if you feel that this issue needs to remain open/active. Thank you for your contributions and help in keeping things tidy!

I did not test this, but I suppose it would be possible to create an empty meta gem (e.g. octokit-faraday-dependencies) that has exactly two versions:

  1. Version 1.0 requires faraday >=1, < 2.
  2. Version 1.1 requires faraday >=2, <3 and additionally faraday-retry (and faraday-multipart, if necessary).
    octokit itself would then require octokit-faraday-dependencies ~> 1.0. By default, the newest faraday 2.x should then be used, unless some other dependency specifies faraday < 2, which would resolve the meta gem to version 1.0.

I don’t really like this solution since it introduces a new gem, but I guess it would work. Unless gems start allowing dependency-version-conditional additional dependencies, I don’t really see any way other than that, requiring faraday >= 2 or the above options (which seem to have been rejected in #1569).