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

seq doesn't work for timezone aware datetime objects #322

Closed
knyghty opened this issue Jun 8, 2022 · 4 comments · Fixed by #353
Closed

seq doesn't work for timezone aware datetime objects #322

knyghty opened this issue Jun 8, 2022 · 4 comments · Fixed by #353
Assignees

Comments

@knyghty
Copy link
Contributor

knyghty commented Jun 8, 2022

Expected behavior

I expect to get a generator of datetimes.

Actual behavior

I got an exception.

Reproduction Steps

>>> from model_bakery.recipe import seq
>>> from datetime import datetime, timedelta, timezone
>>> dt = datetime(2022, 1, 1, tzinfo=timezone.utc)
>>> x = seq(dt, increment_by=timedelta(minutes=1))
>>> next(x)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/opt/venv/lib/python3.10/site-packages/model_bakery/utils.py", line 85, in seq
    start = (date - datetime.datetime(1970, 1, 1)).total_seconds()
TypeError: can't subtract offset-naive and offset-aware datetimes

Versions

Python: 3.10
Django: 4.0.5
Model Bakery: 1.5.0

@HigorMonteiro
Copy link

HigorMonteiro commented Jul 8, 2022

Hello @knyghty
Do you really need to make use of tzinfo=timezone.utc passing it as a parameter?
I ask this question because you can use the timezone set in the django settings see the example below:

>>> from model_bakery.recipe import seq
>>> from datetime import datetime, timedelta, timezone
>>> settings.configure(USE_TZ = True)
>>> dt = datetime(2022, 1, 1)
>>> x = seq(dt, increment_by=timedelta(minutes=5))
>>> next(x)
>>> datetime.datetime(2022, 1, 1, 0, 5, tzinfo=datetime.timezone.utc)

Other example this is https://github.com/model-bakers/model_bakery/blob/main/tests/test_utils.py#L146

@knyghty
Copy link
Contributor Author

knyghty commented Jul 13, 2022

@HigorMonteiro I suppose it's not necessary no, as long as everything comes out in UTC. However I do find this behaviour a bit odd and I would expect it to work with aware datetimes.

I actually find it a bit confusing that it silently converts naive datetimes into aware ones but chokes on already aware datetimes - I find it cleaner to deal with explicitly aware datetimes where possible.

@timjklein36
Copy link
Collaborator

I would tend to agree with @knyghty and it should be a relatively simple fix.

@timjklein36 timjklein36 self-assigned this Oct 13, 2022
berinhard pushed a commit that referenced this issue Oct 14, 2022
* [issue-322] seq increment datetime matching tz-awareness of start date

- This also introduces a test case (additional assert in existing test
  method) to cover the regression

* [issue-322] Update CHANGELOG
@amureki
Copy link
Collaborator

amureki commented Oct 24, 2022

Hey @knyghty !

We released the fix for this issue in model-bakery 1.9.0.
Happy to get your feedback on how this works for you! 🙏

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 a pull request may close this issue.

4 participants