Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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

Open
1 task done
expeehaa opened this issue Apr 25, 2023 · 2 comments · May be fixed by #1706
Open
1 task done

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

expeehaa opened this issue Apr 25, 2023 · 2 comments · May be fixed by #1706
Labels
hacktoberfest Issues for participation in Hacktoberfest Status: Up for grabs Issues that are ready to be worked on by anyone Type: Bug Something isn't working as documented

Comments

@expeehaa
Copy link

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
@expeehaa expeehaa added Status: Triage This is being looked at and prioritized Type: Bug Something isn't working as documented labels Apr 25, 2023
@kfcampbell kfcampbell moved this from 🆕 Triage to 🔥 Backlog in 🧰 Octokit Active May 1, 2023
@kfcampbell kfcampbell added Priority: Normal and removed Status: Triage This is being looked at and prioritized labels May 1, 2023
@nickfloyd nickfloyd moved this from 🔥 Backlog to 👀 In review in 🧰 Octokit Active May 23, 2023
@nickfloyd nickfloyd added the Status: Up for grabs Issues that are ready to be worked on by anyone label Jul 3, 2023
@nickfloyd nickfloyd moved this from 👀 In review to 🔥 Backlog in 🧰 Octokit Active Jul 3, 2023
@nickfloyd nickfloyd added the hacktoberfest Issues for participation in Hacktoberfest label Sep 20, 2023
Copy link

👋 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!

@github-actions github-actions bot added the Status: Stale Used by stalebot to clean house label Jun 17, 2024
@expeehaa
Copy link
Author

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).

@github-actions github-actions bot removed the Status: Stale Used by stalebot to clean house label Jun 18, 2024
@akihikodaki akihikodaki linked a pull request Jul 14, 2024 that will close this issue
4 tasks
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
hacktoberfest Issues for participation in Hacktoberfest Status: Up for grabs Issues that are ready to be worked on by anyone Type: Bug Something isn't working as documented
Projects
Status: 🔥 Backlog
3 participants