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

Issue 68, 67: add additional exception handling to patching code #70

Merged
merged 1 commit into from
Dec 29, 2014

Conversation

mbox
Copy link
Contributor

@mbox mbox commented Dec 29, 2014

Adds more exception handling while patching modules to handle various issues that can arise. I think this will fix #68, #67

@mbox
Copy link
Contributor Author

mbox commented Dec 29, 2014

One question/issue - this uses a minimal approach to exception handling, but arguably it should do pokemon catching since if anything goes wrong while patching/unpatching it's probably better to keep going rather than bomb out and leave things in an odd state

@spulec
Copy link
Owner

spulec commented Dec 29, 2014

I was actually just working on a work project that uses freezegun as a dependency and ran into this. Within a minute of hitting the bug, I checked my email and saw this PR. Sometimes open source is really great.

Thanks for your help!

@spulec
Copy link
Owner

spulec commented Dec 29, 2014

Just saw your question: I think all of those ifs should really be elifs so I don't think it matters. Is that right?

@mbox
Copy link
Contributor Author

mbox commented Dec 29, 2014

I'm more thinking that the code should be:

try:
   if attribute_value == real_datetime:
               setattr(module, module_attribute, FakeDatetime)
   if attribute_value == real_date:
                setattr(module, module_attribute, FakeDate)
   if attribute_value == real_time:
                setattr(module, module_attribute, fake_time)
except:
   pass

rather than being explicit about which exceptions to catch. But agree that it should be if/elif not if/if

@mbox
Copy link
Contributor Author

mbox commented Dec 29, 2014

PS thanks for a nifty tool!

@spulec
Copy link
Owner

spulec commented Dec 29, 2014

Oh, I see.

I'm always a bit nervous to have blanket excepts like that. I think I would rather wait and see if people run into other issues before making the change.

(going to fix the if/elif in master shortly)

Thanks again!

spulec added a commit that referenced this pull request Dec 29, 2014
Issue 68, 67: add additional exception handling to patching code
@spulec spulec merged commit 08c2f88 into spulec:master Dec 29, 2014
@spulec
Copy link
Owner

spulec commented Dec 29, 2014

As soon as I tried the new version with my work project I ran into a RuntimeError so it appears that your suggestion is necessary. Added with 47d113e.

Going to do some more testing, but I will release a new version shortly.

@mbox
Copy link
Contributor Author

mbox commented Dec 29, 2014

Yes, just ran into exactly same issue on my project!

On Monday, 29 December 2014, Steve Pulec notifications@github.com wrote:

As soon as I tried the new version with my work project I ran into a
RuntimeError so it appears that your suggestion is necessary. Added with
47d113e
47d113e
.

Going to do some more testing, but I will release a new version shortly.


Reply to this email directly or view it on GitHub
#70 (comment).

Sent from my phone, please excuse any mistakes.

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.

Having sqlalchemy installed breaks freezegun
2 participants