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

utils: add function now #178

Closed
wants to merge 1 commit into from

Conversation

utnapischtim
Copy link
Contributor

No description provided.

@utnapischtim utnapischtim marked this pull request as draft November 15, 2024 22:07
Comment on lines +137 to +139
def now():
"""Creates a datetime object."""
return datetime.now(timezone.utc).replace(tzinfo=None)
Copy link
Member

@slint slint Dec 16, 2024

Choose a reason for hiding this comment

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

IMHO I would prefer that we find a more "SQLAlchemy way" to avoid this footgun:

  • It's going to be difficult to teach newcomers and (unfortunately, manually) enforce thoughout the codebase doing from invenio_db import now everytime we store UTC/non-TZ-aware datetimes in the DB.
  • It's going to be difficult to catch datetime-related bugs in tests, since often we just "ignore" the values of created/updated fields in responses and focus more on "data content" (which is not a bad thing when applied consiously).
  • I would be more in favour towards finding some config/setting/param in SQLAlchemy datetime columns or even defining a new custom DateTime column type that "does the right thing"™️ on (de)serialization which we can then gradually replace throughout the codebase in models that:
    • Doesn't affect Alembic migrations (e.g. column types in Postgres are still TIMESTAMP WITHOUT TIME ZONE)
    • Makes sure that Python datetime objects with the UTC timezone, get transformed accordingly
    • Explicitly complains/errors when non-UTC timezone-aware datetimes are passed, so that it's not the humans/reviewers that have to check, but the code does it instead.

There's nothing to be done at the moment, but this should be discussed a bit in more detail, so we make a sound decision that we apply.


On a side note, I think we've never explicitly wrote this anywhere, but the common practice throughout Invenio is to store/persist UTC times and convert to/from local timezone-aware times in the UI. I don't know where to log this "decision", but this post is a good supplementary read, from which I get the feeling that for us storing UTC is better.

@utnapischtim
Copy link
Contributor Author

superseded by #179

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.

2 participants