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.

Sign up for GitHub

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

Jump to bottom

Fix on_body issue for Typhoeus w/ code style changes #656

Merged
merged 5 commits into from
Aug 14, 2017
Merged

Fix on_body issue for Typhoeus w/ code style changes #656

merged 5 commits into from
Aug 14, 2017

Conversation

ikapelyukhin
Copy link
Contributor

@ikapelyukhin ikapelyukhin commented Aug 3, 2017 •
edited

#614 with the requested code style changes. Addresses #509.

@ikapelyukhin
Copy link
Contributor Author

ikapelyukhin commented Aug 3, 2017 •
edited

This spec is failing:

rspec ./spec/lib/vcr/library_hooks/typhoeus_spec.rb:86
# Typhoeus hook when used with a typhoeus-based faraday connection records and replays headers correctly

But it's failing in master too.

@ikapelyukhin
Copy link
Contributor Author

@ecnalyr I've implemented the changes you've requested in #614, please take a look.

@mcfiredrill
Copy link
Collaborator

Thanks for your PR.
I'm honestly wondering if we should keep supporting typheous in future versions.
#464 (comment)

@ikapelyukhin
Copy link
Contributor Author

@mcfiredrill why wouldn't you keep it? Typhoeus is a wrapper of libcurl, which probably is the most widely-used HTTP client across different languages and platforms.

The comment you've referenced is talking specifically about dropping Support for Typhoeus <= 0.4. -- the current version of Typhoeus is 1.1.2. Dropping support for legacy versions isn't quite the same as dropping support for it completely. :-)

I've used the code in this pull request and have been able to record the cassette successfully.

@mcfiredrill
Copy link
Collaborator

@ikapelyukhin
Thanks for clearing that up! I overlooked that the comment was talking about an old version.

I'm OK to merge this but any idea why the build is failing?

@ikapelyukhin
Copy link
Contributor Author

@mcfiredrill I'm not sure. As I've mentioned -- the same spec seems to fail in master as well (that's why I haven't looked into it).

@mcfiredrill
Copy link
Collaborator

@ikapelyukhin I will try to get master passing

@ikapelyukhin
Copy link
Contributor Author

ikapelyukhin commented Aug 4, 2017 •
edited

I've fixed a couple of other issues:

  • Response body wasn't recorded if a there was an on_body callback that returned :abort
  • on_headers and on_body callbacks weren't invoked when playing back the cassette

@ikapelyukhin
Copy link
Contributor Author

@mcfiredrill I've fixed the issue with the broken Faraday spec in #657.

This was referenced Aug 7, 2017
Closed
Merged
sazor and others added 5 commits August 14, 2017 12:12
@sazor @ikapelyukhin
Fix on_body issue for Typhoeus
683999f
@ikapelyukhin
Add newlines to Typhoeus files
62762b4
@ikapelyukhin
Fix cassette playback/recording for Typhoeus
9a3cef0 
- Made sure that on_body callback to record the cassette is invoked first, otherwise if there's a callback before it that returns :abort then the body will not be recorded
- Will now invoke on_header and on_body callbacks when playing back the cassette
@ikapelyukhin
Add specs for Typhoeus callbacks
faa3a7b
@ikapelyukhin
Clean up the spec slightly
2ecef09
@ikapelyukhin
Copy link
Contributor Author

Rebased the branch, let's see if any tests flicker again.

@ikapelyukhin
Copy link
Contributor Author

@mcfiredrill All green!

@mcfiredrill mcfiredrill merged commit 0ce29d0 into vcr:master Aug 14, 2017
@mcfiredrill
Copy link
Collaborator

Looking good, thanks for your work!

@ikapelyukhin
Copy link
Contributor Author

@mcfiredrill no problem. Any plans for the next version release?

@mcfiredrill
Copy link
Collaborator

We've been due for one for a bit, I will get to releasing 4.0 as soon as I can. 🙇

@mcfiredrill
Copy link
Collaborator

mcfiredrill commented Aug 15, 2017 •
edited

BTW I had a TODO list for 4.0 but I think everything is basically done.
#578

@ikapelyukhin
Copy link
Contributor Author

Sounds good! 👍

@ikapelyukhin ikapelyukhin deleted the issue_509 branch August 30, 2017 10:45
@ikapelyukhin
Copy link
Contributor Author

@mcfiredrill any word on 4.0 release? Need a hand with anything, maybe? 😬

zeisler added a commit to zeisler/vcr that referenced this pull request Jun 15, 2020
@zeisler
Add support for on_data with faraday.
4ee2db3 
Followed style of vcr#656 for Typhoeus.
@zeisler zeisler mentioned this pull request Jun 15, 2020
Merged
zeisler added a commit to zeisler/vcr that referenced this pull request Jun 15, 2020
@zeisler
Add support for on_data with faraday.
8ae889b 
Followed style of vcr#656 for Typhoeus.
olleolleolle pushed a commit that referenced this pull request Mar 13, 2022
@zeisler
Add support for on_data with faraday. (#823)
536443e 
Followed style of #656 for Typhoeus.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ikapelyukhin @mcfiredrill @sazor