fix ignoring packages that are imported after freezing

[pegler]

When a package is imported after freezegun starts, ignore would not work correctly. This fixes FakeDate.today(), FakeDatetime.now() and FakeDatetime.utcnow() to return the real date/datetime in ignored packages.

fixes #420

[pegler]

hi @boxed and @spulec, thanks for your efforts in maintaining this package. We've found it incredibly useful, but unfortunately identified this bug. Right now we're patching freezegun to make it work correctly and it'd be great if we could get the fix applied to the package itself. Let me know if there are any changes you'd like to see. Thanks!

[immerrr]

Hi,

Just ran into this because S3 was rejecting requests made from within freeze_time block as too old, and was unable to ignore my way around it. Turns out, boto3 uses import datetime; datetime.datetime instead of from datetime import datetime, and that is enough for ignores to be ignored.

It is easy to show that this is broken with a small patch against existing tests:

5c305e6

And I came up with a similar fix:

7fa6515

Would be great to have this fixed upstream 👍 Thanks!

[immerrr]

@boxed would you be able to have a look at this? it is still impossible to use freezegun with any code that's uploading files to S3, because of #331, and this PR would fix it.

[pegler]

@immerrr I switched to https://github.com/adamchainz/time-machine a while ago and have been very happy. You need to take a slightly different approach to handling ignores, but it worked well for our project.

[boxed]

I don't understand what this added logic is for. cls._date_to_freeze() calls get_current_time() which already calls _should_use_real_time() and does this type of thing so this change seems redundant?

freezegun/freezegun/api.py

Line 174 in 92624e4

if _should_use_real_time():

[immerrr]

@boxed I'm happy to submit my change which should be smaller if that would make it easier to get this in.

I could add the test case from this PR to make sure it covers the issue mentioned in this PR, too.

[boxed]

@immerrr that looks wrong to me too honestly. It's too complex. Something is fishy.

[immerrr]

Just to make sure we are talking about the same thing, you mean my patch that touches these two conditionals is too complex, right?

[boxed]

Yes. Correct. I feel like this logic should be fully contained within get_current_time itself. It just wouldn't be DRY otherwise, and it would also mean there's a ton of duplicate code and every place there isn't duplication would be a bug.

[immerrr]

There are also performance considerations here, e.g. some blocks like

if _should_use_real_time():
    return call_some_builtin_function()

may work much faster than converting the result of get_current_time() back to the required format.

DRY is a valid principle, but repetition only manifests itself if the code needs to change. If it is a small module that is feature complete, what difference does it make if the code repeats one time, two times or ten times, the interpreter doesn't care.

That said, let me see if I can hide should_use_real_time inside get_current_time...

[boxed]

may work much faster than converting the result of get_current_time() back to the required format.

There is no conversion in this code though.

That said, let me see if I can hide should_use_real_time inside get_current_time..

...but get_current_time already calls should_use_real_time! That's my point.

[immerrr]

There is no conversion in this code though.

There are conversions in functions like fake_monotonic whose "faking" versions use get_current_time

freezegun/freezegun/api.py

Lines 221 to 225 in be4127b

	def fake_monotonic():
	if _should_use_real_time():
	return real_monotonic()
	return _get_fake_monotonic()

So if we continue with the DRY idea of moving _should_use_real_time inside get_current_time, functions like fake_monotonic will look like

def fake_monotonic():
    current_time = get_current_time()
    return (
        calendar.timegm(current_time.timetuple()) +
        current_time.microsecond / 1e6
    )

which means they would take a performance hit even in cases when are using the "real" function. Do you have some other idea in mind?

but get_current_time already calls should_use_real_time!

I don't think it does, or maybe i'm looking somewhere else

freezegun/freezegun/api.py

Lines 169 to 170 in be4127b

	def get_current_time():
	return freeze_factories[-1]()

[boxed]

@pegler that's cool! I've been meaning to try it, just haven't had time to check it out.

As everyone here is painfully aware, neither me not spulec have the energy/motivation to maintain freezegun to the level it deserves as a very widely used library. So if time-machine is actually a viable replacement, that's very good to hear.