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 cassette context decorator nesting issues #108

Conversation

colonelpanic8
Copy link
Collaborator

Sorry for spamming you with pull requests.
This is getting kind of big (it looks works than it is, I promise) so just to make sure that we are keeping track, here is a list of all of the changes that are included in this pull request:

  • Move context management of use_cassette into CassetteContextDecorator so that a separate cassette is created for each invocation of a use_cassette decorated function.
  • VCR now passes a callable to CassetteContextDecorator (instead of passing the arguments for cassette creation) so that changes to the VCRs default configuration can propagate to functions that have already been decorated.
  • Add mock and contextlib2 as dependencies.
  • Do all patching through mock.patch. This made it so that CassetteContextDecorators actually stack properly.
  • Remove enter and exit from Cassette class.
  • Add before_record_response hook to Cassette and VCR classes. This is mainly for the purpose of scrubbing responses.
  • Version bump (1.0.3 -> 1.0.4). Should this be a minor version bump instead?
  • Remove contextdecorator dependency.

closes #102, #107, #109

@colonelpanic8 colonelpanic8 force-pushed the fix_CassetteContextDecorator_nesting_issues branch from 2442115 to 5edc58f Compare September 18, 2014 14:10
@colonelpanic8
Copy link
Collaborator Author

lol @ three builds in a row failing. I have a good feeling about e1e08c7 though. I think it might actually pass.

@colonelpanic8 colonelpanic8 force-pushed the fix_CassetteContextDecorator_nesting_issues branch from 76c185e to 4868a63 Compare September 18, 2014 21:43
…re_first_nesting. by using a single class and patching cassette on that class. Not a great solution :\
@colonelpanic8
Copy link
Collaborator Author

8947f0f is going to fail because it has a the test from #109. Solving that one is sort of tricky, and it could require a little bit of gross surgery on VCRConnection (or a major refactor).

@kevin1024
Copy link
Owner

Sorry for spamming you with pull requests.

No need to apologize, it's great. In fact, are you interested in helping me maintain vcrpy?

Do all patching through mock.patch. This made it so that CassetteContextDecorators actually stack properly.

Wow, that's another use case I never thought about before. Yeah, I guess having it work like a stack is the right way to do it.

Add before_record_response hook to Cassette and VCR classes. This is mainly for the purpose of scrubbing responses.

OK, I could see this being useful. You would need to add a section to the README about how to use this feature, and the difference from before_record

Version bump (1.0.3 -> 1.0.4). Should this be a minor version bump instead?

Definitely a minor version bump, we are adding new features.

@colonelpanic8
Copy link
Collaborator Author

Yeah I'd love to help you out with the maintenance of vcrpy! What exactly would that entail?

Good call on adding a section to the README for before_record_reponse. I'm going to take a swing at #109, but I sort of suspect that it could be a bit tricky so I might give up on getting it into this version. In that case I'd like to leave the test that I wrote for it in, so is it cool if I disable it for now with the reason set to the issue number?

I don't think the issue is THAT serious because the use case of using a session across contexts is somewhat obscure, but the behavior is pretty surprising and I can contrive a few situations where you might want to do something like that.

@colonelpanic8
Copy link
Collaborator Author

Also, how do you feel about adding https://github.com/mverteuil/pytest-ipdb (or just raw ipdb) as a py.test plugin. ipdb is just so much better than raw pdb.

@kevin1024
Copy link
Owner

Yeah I'd love to help you out with the maintenance of vcrpy! What exactly would that entail?

I just gave you commit access, just go ahead and fix bugs. If it's a new feature or a backwards incompatible change, run it by me first though. You'll probably have to ping me to upload to pypi if you want to do a new release.

I'd like to leave the test that I wrote for it in, so is it cool if I disable it for now with the reason set to the issue number?

Yes, sounds good to me, you can mark it xfail

I don't think the issue is THAT serious because the use case of using a session across contexts is somewhat obscure, but the behavior is pretty surprising and I can contrive a few situations where you might want to do something like that.

Yup, it's a pretty bizarre bug, but I agree, it would be nice to have a fix.

@colonelpanic8
Copy link
Collaborator Author

Cool, thanks! Do you still want me to merge with --no-ff for the sake of record keeping or are you okay with changes pushed directly to master?

@kevin1024
Copy link
Owner

You can just push to master if you want, the changes in this pull request aren't exactly related anyway (new before_save_request feature, bugfixes, etc)

…ted_before_first_nesting. by using a single class and patching cassette on that class. Not a great solution :\"

This reverts commit 2bf23b2.
@colonelpanic8
Copy link
Collaborator Author

Cool. that sounds good. I think I have to push all of this together just because I've been touching a lot of the same files in the various commits ive had. I'm hoping to try to get everything done somewhat soon. If you could help me get the new version into pypy that would be great. Is there anything else that you'd want to get into a new version?

@kevin1024
Copy link
Owner

Sure, just ping me when you're ready. A few things I like to do for releases:

  1. Make sure all the tests are passing :)
  2. Add a note to the changelog in the README.md
  3. Make a new release on Github, this will tag it too. tags look like vx.x.x
  4. Push to pypi.

Is there anything else that you'd want to get into a new version?

I think this version has plenty (thanks for all your work!) - some bugfixes and also a new feature.

@colonelpanic8 colonelpanic8 force-pushed the fix_CassetteContextDecorator_nesting_issues branch from 9d54e96 to 2fd21e4 Compare September 20, 2014 17:55
…md with description of `before_record_response`
@colonelpanic8 colonelpanic8 force-pushed the fix_CassetteContextDecorator_nesting_issues branch from 2fd21e4 to 83211a1 Compare September 20, 2014 18:07
…ven if import fails. Make _recursively_apply_get_cassette_subclass actually work with dictionaries.
…onnections in ConnectionPools."

This reverts commit dc249b0.

Conflicts:
	vcr/patch.py
@colonelpanic8 colonelpanic8 force-pushed the fix_CassetteContextDecorator_nesting_issues branch from 55f3fa3 to 113c95f Compare September 21, 2014 12:16
colonelpanic8 added a commit that referenced this pull request Sep 21, 2014
…_nesting_issues

Fix cassette context decorator nesting issues
@colonelpanic8 colonelpanic8 merged commit 3dea853 into kevin1024:master Sep 21, 2014
@colonelpanic8 colonelpanic8 deleted the fix_CassetteContextDecorator_nesting_issues branch September 21, 2014 12:35
@colonelpanic8
Copy link
Collaborator Author

Hey. I wasn't able to push to pypi because I don't have access. Can you do this for me?

@kevin1024
Copy link
Owner

OK, pushed to pypi. Thanks for your work!

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