-
Notifications
You must be signed in to change notification settings - Fork 982
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
Introduce Middleware DEFAULT_OPTIONS with Application and Instance Configurability #1572
Introduce Middleware DEFAULT_OPTIONS with Application and Instance Configurability #1572
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 💪
@iMacTia good catch on the typo 🤦 , and thanks for the feedback! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great, congrats on your first Faraday contribution 🙌
This was very well executed 💯
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 🚀 |
Description
This PR introduces a new feature to Faraday::Middleware: Default Options. While the concept of
DEFAULT_OPTIONS
isn'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_OPTIONS
to be set at theFARADAY::MIDDLEWARE
level, 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 😆