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

Handle rest_framework.throttling.SimpleRateThrottle #459

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Handle rest_framework.throttling.SimpleRateThrottle #459

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f92dd37
test(rest_framework.throttling.SimpleRateThrottle): reproduce the error
JulianMaurin Jul 19, 2022
4313a88
fix(fake_time): accept any args/kwargs
JulianMaurin Jul 19, 2022
9d2b92f
refacto(test_class_import): factorize tests introducting the _date_to…
JulianMaurin Jul 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion freezegun/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ def get_current_time():
return freeze_factories[-1]()


def fake_time():
def fake_time(*args, **kwargs):
Copy link
Contributor

Choose a reason for hiding this comment

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

Why *args, **kwargs here? That seems very wrong to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @boxed ,
Thanks for your review. May I humbly ask you to elaborate?

I think I made an effort to explain the reproduction of the bug, both in the description and in the test.
If I'm off track, can you possibly guide me to a solution that you think is more appropriate?

Copy link
Contributor

Choose a reason for hiding this comment

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

@JulianMaurin this is breaking the API typing on api.pyi, so users that have mypy will have problems. I also think you can add typed arguments to satisfy your needs with default arguments=None so it will also not break current applications and maintain the types well defined.

Copy link
Contributor Author

@JulianMaurin JulianMaurin Aug 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I thought this version does not break the application, in addition, fake_time function is not defined in api.pyi, but maybe I did not understand something.

Would you prefer this version (with a docstring ofc):

Suggested change
def fake_time(*args, **kwargs):
def fake_time(_ : Any):

Copy link
Contributor

Choose a reason for hiding this comment

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

@JulianMaurin putting an argument of type Any makes the method still untyped. Also as @boxed adding *args, **kwargs here does not make sense to me also, since to fix the bug you one need to be able to call time it with self.

You can check in the definition of builtin time.time there is NO argument whatsoever: https://github.com/python/cpython/blob/v3.9.6/Modules/timemodule.c#L86

Have to tried, instead of adding arguments, wrapping fake_time with @classmethod? Maybe it will fix this usage. If not I am sure some kind of wrapper like this will be needed.

if _should_use_real_time():
return real_time()
current_time = get_current_time()
Expand Down
9 changes: 6 additions & 3 deletions tests/test_class_import.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,12 +134,15 @@ def test_fake_strftime_function():
assert fake_strftime_function() == '2012'


@freeze_time("2022-01-01 12:00:00")
@freeze_time("2022-01-01")
def test_fake_time_function_as_class_attribute():
Copy link
Contributor

Choose a reason for hiding this comment

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

It is great that you added tests, but it is also important to explain on README the new optional usage in English so more people can easily reuse your solution =)

Copy link
Contributor

Choose a reason for hiding this comment

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

You can also add some description of this change on CHANGELOG.

local_time = datetime.datetime(2022, 1, 1)
utc_time = local_time - datetime.timedelta(seconds=time.timezone)
expected_timestamp = time.mktime(utc_time.timetuple())
JulianMaurin marked this conversation as resolved.
Show resolved Hide resolved

fake_module_lazy.load()

with pytest.raises(TypeError, match='fake_time\(\) takes 0 positional arguments but 1 was given'):
assert fake_module_lazy.TimeAsClassAttribute().call_time()
assert fake_module_lazy.TimeAsClassAttribute().call_time() == expected_timestamp


def test_import_after_start():
Expand Down