Introduce Middleware DEFAULT_OPTIONS with Application and Instance Configurability #1572
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
iMacTia
requested changes
Jul 5, 2024
There was a problem hiding this comment.
Hi @ryan-mcneil, thank you for the thorough PR (complete with extensive documentation, much appreciated 🙏) and apologies if it took me longer than expected to review.
This looks really great and implements pretty well what I had in mind and described in the comments you linked to.
I requested a couple changes but this looks like a really solid start 💪
Co-authored-by: Matt <iMacTia@users.noreply.github.com>
|
@iMacTia good catch on the typo 🤦 , and thanks for the feedback! |
iMacTia
approved these changes
Jul 5, 2024
|
Release note: I'll be away most of the weekend so I'll cut a release on Monday if that's OK as I would hate to be unavailable in case anything goes wrong |
|
Shoutout to @ericboehs for the assist! Thanks @iMacTia! Looking forward to it! |
|
This has just been released in v2.10.0 🚀 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR introduces a new feature to Faraday::Middleware: Default Options. While the concept of
DEFAULT_OPTIONSisn't fundamentally new to Faraday, this PR provides some building blocks to introduce Default Options, and the ability to override them at the application level. While it has only been implemented in the RaiseError class, it can easily be added to other Faraday::Middleware classes for which there's a need.The suggestion was first mentioned here with some conversation and ideation afterwards. As our team did have a need for this functionality (i.e. setting a different default for
:include_request), I created a little monkeypatch in our repo, and figured I'd submit a [much more thought out] addition to Faraday itself.My addition to the documentation (please let me know if you would like this elsewhere!) mostly sums up the general idea and how a user would implement it.
Addresses comment here
Additional Notes
The additional "feature" that I did not include in the documentation is that this implementation allows for
DEFAULT_OPTIONSto be set at theFARADAY::MIDDLEWARElevel, that would be propagated to child classes. Leaving it blank for now allows for the feature to work as intended, but could open it up for additional configuration in future iterations of the gem. I'd love to hear your thoughts here.This is my first time contributing to a widely used gem, so be gentle 😆