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

Replace timezone code by Django's built in solution #35

Closed
5 tasks
anapaulagomes opened this issue Nov 17, 2019 · 8 comments
Closed
5 tasks

Replace timezone code by Django's built in solution #35

anapaulagomes opened this issue Nov 17, 2019 · 8 comments
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@anapaulagomes
Copy link
Contributor

anapaulagomes commented Nov 17, 2019

Model Bakery has a module to deal with timezones that was implemented before Django had a built in support for it. We should replace it by the timezone API from Django 1.11.

https://github.com/model-bakers/model_bakery/blob/c2b609f6b065e587cf4a2e4253b518634d4995b3/model_bakery/timezone.py

Expected behavior

This lib should use Django's built in timezone library instead of bakery's timezone module.

Things I could think of:

  • Replace calls to smart_datetime
  • Replace calls to tz_aware
  • Replace calls to now
  • Replace calls to utc
  • Delete bakery's timezone module
@anapaulagomes anapaulagomes added enhancement New feature or request good first issue Good for newcomers labels Nov 17, 2019
@dubesar
Copy link

dubesar commented Nov 17, 2019

@anapaulagomes By delete timezone module you mean to delete timezone.py?

@anapaulagomes
Copy link
Contributor Author

Yes, @dubesar. I just updated the text to make it more explicit.

@dubesar
Copy link

dubesar commented Nov 17, 2019

@anapaulagomes So basically all the imports of timezone.py in all the other modules have to be replaced and also a single line of django for timezone is to be included.

@anapaulagomes
Copy link
Contributor Author

I can't tell for sure that these are the only things needed here. Yes, this issue is mainly about replacing the methods from model_bakery/timezone.py by Django's timezone module. But we have to make sure that the library still works the same way as before having the module replaced, which may include to adapt some things. @dubesar

@dubesar
Copy link

dubesar commented Nov 17, 2019

@anapaulagomes what I am thinking to do is :
1.
Replace :

from datetime import datetime

try:
    from django.conf import settings
    from django.utils.timezone import now, utc
except ImportError:
    def now():
        return datetime.now()

with

from django.utils import timezone
import datetime

def now():
    """
    Returns an aware or naive datetime.datetime, depending on settings.USE_TZ.
    """
    if settings.USE_TZ:
        # timeit shows that datetime.now(tz=utc) is 24% slower
        return datetime.utcnow().replace(tzinfo=utc)
    else:
        return datetime.now()

Remove tz_aware as now is sufficient and also remove smart_datetime

Replace :

series_date = tz_aware(datetime.datetime.utcfromtimestamp(start + n))

with

series_date = now()+n

And there seems to be no change in random_gen.py

@dubesar
Copy link

dubesar commented Nov 17, 2019

@anapaulagomes If you find these changes correct then ping me so that I can make changes in the repo

@anapaulagomes
Copy link
Contributor Author

A good way of starting this is making all the changes needed in model_bakery/timezone.py and expect the tests to pass. After calling Django's methods from bakery's methods and making sure that everything is working fine, you'll be fine to replace the calls all over the code base. @dubesar

For the example you've shown, I'd say that just from django.utils.timezone import now would be enough. From Django's docs:

When time zone support is enabled (USE_TZ=True), Django uses time-zone-aware datetime objects. If your code creates datetime objects, they should be aware too. In this mode, the example above becomes:
from django.utils import timezone
now = timezone.now()

To test now()'s case you only need to remove its try/except and then run the tests. This try/except was there to make sure that a code from before Django 1.4 would work. So now instead of returning datetime.now() we'd be just importing Django's method there.

From:

try:
    from django.conf import settings
    from django.utils.timezone import now, utc
except ImportError:
    def now():
        return datetime.now()

To:

from django.conf import settings
from django.utils.timezone import now, utc

Same goes to tz_aware and smart_datetime. Please make sure tests are passing. :)

Any thoughts here, @amureki?

amureki pushed a commit that referenced this issue Jun 13, 2020
amureki pushed a commit that referenced this issue Sep 4, 2020
@anapaulagomes anapaulagomes linked a pull request Oct 14, 2020 that will close this issue
amureki pushed a commit that referenced this issue Feb 10, 2021
And use `django.utils.timezone.now` instead.
Refs #35.
amureki pushed a commit that referenced this issue Feb 10, 2021
And use `django.utils.timezone.now` instead.
Refs #35.
amureki pushed a commit that referenced this issue Feb 17, 2021
* Modify `setup.py` to not import the whole module for package data

* mypy to run against lib only

* Drop obsolete `model_bakery.timezone.now`

And use `django.utils.timezone.now` instead.
Refs #35.
amureki pushed a commit that referenced this issue Feb 27, 2021
- remove `smart_datetime` (replaced by direct `tz_aware` use)
- update module docstrings, remove TODO comment
amureki added a commit that referenced this issue Feb 27, 2021
* Add a unit test for `utils.seq`

Previously, we only tested it within recipe tests.

* Update changelog

* Fix USE_TZ test

* Clean up `model_bakery/timezone.py` (refs #35)

- remove `smart_datetime` (replaced by direct `tz_aware` use)
- update module docstrings, remove TODO comment

* Update changelog
@amureki
Copy link
Collaborator

amureki commented Feb 27, 2021

Done in:
#141
#143
#147

Please, note, I left model_bakery.timezone. tz_aware - I think, this is good to have. However, I am happy to reopen this if anyone would be having suggestions.

@amureki amureki closed this as completed Feb 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants