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

allow private parse method in response middleware #1123

Merged
merged 5 commits into from
Feb 10, 2020
Merged

allow private parse method in response middleware #1123

merged 5 commits into from
Feb 10, 2020

Conversation

nic-lan
Copy link
Contributor

@nic-lan nic-lan commented Feb 5, 2020

Description

Currently the Response Middleware enforces children classes to define public parse method.
This is due to the implicit arg all_included=false passed to respond_to? method

Now I would like to allow also private and protected parse method to allow more flexibility for the children classes.

Todos

List any remaining work that needs to be done, i.e:

  • Tests
  • Documentation

Additional Notes

When running bundle exec rspec on master i noticed a bounce of failing specs which are not depending on this PR code changes.

The error is:

Failure/Error: @session.request(env[:url].to_s, rack_env)

      NoMethodError:
        undefined method `split' for nil:NilClass

and it refers to lib/faraday/adapter/rack.rb:62

Maybe an issue to fix that should be open.

Copy link
Member

@iMacTia iMacTia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although I'm not entirely sure as why someone would want to define the parse method as private, I have nothing against it in principle, so happy to merge this in.
I do, however, not entirely like the unnecessary line I commented on below, and would prefer to take it out before merging.

lib/faraday/response.rb Outdated Show resolved Hide resolved
@nic-lan
Copy link
Contributor Author

nic-lan commented Feb 10, 2020

change as you required.

Let me know if you have anything else that does not convince you.

Copy link
Member

@olleolleolle olleolleolle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All changes implemented, tests pass!

Copy link
Member

@iMacTia iMacTia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for applying the change! Looks good now 🙌
LGTM

@iMacTia iMacTia merged commit 1042a45 into lostisland:master Feb 10, 2020
@nic-lan nic-lan deleted the allow-private-parse-for-response-middleware branch February 10, 2020 21:39
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.

3 participants