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.

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

Make faraday retry fully optional. #1569

Closed

Conversation

expeehaa
Copy link

@expeehaa expeehaa commented May 1, 2023

Resolves #1567


Behavior

Before the change?

When loading the gem with faraday > 2 and faraday-retry not installed, a warning is printed.

After the change?

When loading the gem with faraday > 2 and faraday-retry not installed, no warning is printed.

Other information

This is one of the possible solutions for #1567, specifically the one I prefer. I am well aware that the maintainers could prefer another one and, if so, I could modify the pull request accordingly.


Additional info

Pull request checklist

  • Tests for the changes have been added (for bug fixes / features)
    • kind of N/A since this has not been tested before
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Added the appropriate label for the given change

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

  • Yes (Please add the Type: Breaking change label)
  • No

If Yes, what's the impact:

  • N/A

Pull request type

Please add the corresponding label for change this PR introduces:

  • Bugfix: Type: Bug
  • Feature/model/API additions: Type: Feature
  • Updates to docs or samples: Type: Documentation
  • Dependencies/code cleanup: Type: Maintenance

@nickfloyd
Copy link
Contributor

Hey @expeehaa,

Thanks for the change here. I'm still trying to work out my thoughts on how we handle this; you make a good point about the noise.

If you're using Faraday >= 2.x the gem file specifies that faraday-multipart and faraday-retry should be dependencies

Meaning that you'd have to use the middleware (built-in) for retry if you were using @ >= 1.20 (I belive) and < 2. Or you would use the separate retry gem if you were >= 2.x - I can see how that implementation is confusing.

In either case, we should probably require retry either through the middleware or with the separate gem - but the support and implementation has to be conditional. The reason is, we don't know which one is being used - which is why you get the warning.

I'm am currently at a loss as to how to handle this better. Removing support for Faraday v1 seems like a bit of a drastic change (we cap the version at 3.x) - this is a bit of a precursor to that. I'm going to chat with @kfcampbell about this as well to see if we can come up with a solution, I just really do not have a good one right now.


A note about your change that is orthogonal to our discussion here:

Specifically, on this change, you'll need to suppress the cop exception by adding:
- 'lib/octokit/default.rb'

To the SupressedException list in this file: .rubocop_todo.yml

Or restructure the loading of the gem some other way.

@expeehaa expeehaa force-pushed the make_faraday_retry_fully_optional branch from e901b16 to 08a910a Compare May 24, 2023 00:02
Copy link

👋 Hey Friends, this pull request 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 Feb 18, 2024
@github-actions github-actions bot closed this Feb 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Stale Used by stalebot to clean house
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

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