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.

Sign up for GitHub

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

Jump to bottom

[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:

octokit.rb/lib/octokit/default.rb

Lines 36 to 42 in fb01ac2

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
@expeehaa expeehaa mentioned this issue May 1, 2023
Closed
5 tasks
@kfcampbell kfcampbell added Priority: Normal and removed Status: Triage This is being looked at and prioritized labels May 1, 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 added the hacktoberfest Issues for participation in Hacktoberfest label Sep 20, 2023
@github-actions GitHub Actions
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
Open
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
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
🧰 Octokit Active
Status: 🔥 Backlog
Milestone
No milestone
3 participants
@nickfloyd @kfcampbell @expeehaa