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

Fix scope breaking change of request/response interception. (#241) #242

Merged
merged 1 commit into from
May 12, 2018

Conversation

Jack12816
Copy link
Contributor

Copy link
Contributor

@AlanFoster AlanFoster left a comment

Choose a reason for hiding this comment

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

Does this change mean we can get rid of the extend rspec matcher logic that was originally added to the tests in the original PR? 🤔

@Jack12816
Copy link
Contributor Author

Ah, you mean subject.extend(RSpec::Matchers). Mhh lets see.

@Jack12816 Jack12816 force-pushed the fix-interception branch from 79cd335 to 5c01177 Compare May 1, 2018 09:48
@Jack12816
Copy link
Contributor Author

Yes, this is fine without due to the calling scope. I removed these calls. Thanks.

# method if you like to. This is just a shortcut.
def self.pass_request(params, headers, body, url, method)
handler = proxy.request_handler.handlers[:proxy]
response = handler.handle_request(method, url, headers, body)
Copy link
Contributor

@AlanFoster AlanFoster May 1, 2018

Choose a reason for hiding this comment

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

nab: It's interesting to see the difference between internet/external APIs here.

i.e. the method args are: params, headers, body, url, method - but we're calling internally with method, url, headers, body

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes because of:

proxy.stub('http://example.com/').and_return(Proc.new { |*args|
  response = pass_request(*args)
end

Copy link
Contributor Author

@Jack12816 Jack12816 May 2, 2018

Choose a reason for hiding this comment

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

Makes it easier to use here without introduce a breaking change (on the order of and_return arguments). Its more likely to intercept the response and modify it as to tweak the request. It was fine in first place, no need to debate on it in a fixup.

# the request beforehand and/or modify the actual response which is passed
# back by this method. But you can also implement a custom proxy passing
# method if you like to. This is just a shortcut.
def self.pass_request(params, headers, body, url, method)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to confirm, is params a dead variable here?

@AlanFoster
Copy link
Contributor

Looks good to me, happy to ship this @ronwsmith? :)

@Jack12816
Copy link
Contributor Author

@ronwsmith Ping?

@ronwsmith ronwsmith merged commit a7b50c5 into oesmith:master May 12, 2018
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