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

Configurable JSON encoders and decoders #286

Closed

Conversation

ne006
Copy link

@ne006 ne006 commented Dec 15, 2023

By default FaradayMiddleware::EncodeJson and FaradayMiddleware::ParseJson uses json to encode/decode JSON data, although there are gems like oj which do the job faster than Ruby's built-in json.

This implementation allows to pass encoder option to FaradayMiddleware::EncodeJson and decoder option to pass in parser_options to FaradayMiddleware::ParseJson

require 'oj'

Faraday.new do |conn|
  conn.request :json, encoder: Oj
  conn.response :json, parser_options: { decoder: Oj }
end

@ne006 ne006 marked this pull request as ready for review December 15, 2023 09:16
@ne006 ne006 force-pushed the feature/json-encoders-and-decoders branch from 1c18c09 to 8311642 Compare December 15, 2023 09:21
@iMacTia
Copy link
Member

iMacTia commented Dec 18, 2023

Hi @ne006 and thank you for proposing this contribution.
I like the idea and I've heard similar requests in the past as well.
However, please note that as stated in the README faraday_middleware is now deprecated, so we're not accepting new feature PRs against it.

The EncodeJson and ParseJson middleware are now shipped out-of-the-box with Faraday, so if you'd like you can "move" this contribution directly to them 👍 . See the json request and json response middleware in Faraday.

If you reopen this PR against those, I'll be happy to review.
Based on a quick review, I can say the PR looks good and has good test coverage, but we'd require the documentation to be updated as well before it can be merged, especially around the functionality of passing a class/method pair I see you implemented. You can find the json middleware (both request and response) documentation here.

Thank you again for the contribution and I look forward to reviewing it once moved on Faraday.
For now, I'll be closing this PR for the reason mentioned at the beginning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants