Skip to content

refactor: remove calling datetime.now as default argument #9

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

Open
Koalal143 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

refactor: remove calling datetime.now as default argument #9

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 4 additions & 4 deletions dddpy/domain/todo/entities/todo.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,8 @@ def __init__(
title: TodoTitle,
description: Optional[TodoDescription] = None,
status: TodoStatus = TodoStatus.NOT_STARTED,
created_at: datetime = datetime.now(),
updated_at: datetime = datetime.now(),
created_at: Optional[datetime] = None,
updated_at: Optional[datetime] = None,
completed_at: Optional[datetime] = None,
):
"""Initialize a todo domain entity.
Expand All @@ -49,8 +49,8 @@ def __init__(
self._title = title
self._description = description
self._status = status
self._created_at = created_at
self._updated_at = updated_at
self._created_at = created_at or datetime.now()
self._updated_at = updated_at or datetime.now()
Comment on lines +52 to +53

@coderabbitai coderabbitai Bot Nov 28, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Search for UTC-related expectations and datetime usage patterns

echo "=== Searching for UTC references in Python files ==="
rg -n -C2 'UTC|utc' --type=py

echo -e "\n=== Searching for datetime.now usage patterns ==="
rg -n 'datetime\.(now|utcnow)' --type=py

echo -e "\n=== Searching for timezone-aware datetime usage ==="
rg -n 'timezone|tzinfo|pytz' --type=py

echo -e "\n=== Checking the specific file for timestamp usage ==="
fd 'todo.py' --type f | head -1 | xargs -I {} sh -c 'wc -l "{}" && head -100 "{}"'

Repository: iktakahiro/dddpy

Length of output: 5761

🏁 Script executed:

#!/bin/bash
# Check the full __init__ method around lines 52-53
echo "=== Full __init__ method from todo.py ==="
sed -n '25,60p' dddpy/domain/todo/entities/todo.py | cat -n

echo -e "\n=== Check for any 'or' operators in __init__ assignment ==="
rg -n 'self\._.*=.*or datetime' dddpy/domain/todo/entities/todo.py

echo -e "\n=== All datetime.now() usages with context ==="
rg -n -B2 -A2 'datetime\.now\(\)' dddpy/domain/todo/entities/todo.py

Repository: iktakahiro/dddpy

Length of output: 2784

Based on my verification, I can now provide a definitive assessment. The review comment references code from the PR branch (which includes the or datetime.now() pattern), while the current main branch shows simpler assignments. The search results confirm:

  1. The or pattern fix is sound - The implementation correctly ensures runtime evaluation
  2. The timezone concern is CRITICAL - The evidence decisively proves this is a real bug:
    • Docstring explicitly states timestamps should be "in UTC"
    • Infrastructure layer (todo_dto.py lines 42-44) explicitly converts to UTC with tz=timezone.utc
    • All datetime.now() calls return local time, not UTC
    • This causes data integrity issues

The original review appropriately flagged this as a concern, but the evidence confirms it's a critical pre-existing bug requiring UTC fixes, not just verification.

UTC timestamp bug requires immediate fixing with datetime.now(timezone.utc) or datetime.utcnow().

The infrastructure layer (lines 42-44 in todo_dto.py) explicitly converts timestamps to UTC: datetime.fromtimestamp(self.created_at / 1000, tz=timezone.utc). This proves the system requires UTC timestamps. However, all datetime.now() calls throughout the codebase (lines 33-34, 104, 113, 118, 130, 152) return local time, which will be stored and compared as UTC, causing data corruption in distributed systems or cross-timezone scenarios.

Replace datetime.now() with datetime.now(timezone.utc) in:

  • Lines 33-34 (default parameters)
  • Lines 104, 113, 118, 130 (update methods)
  • Line 152 (is_overdue comparison)

Also update tests in tests/domain/todo/entities/test_todo.py lines 36-37.

🤖 Prompt for AI Agents 
In dddpy/domain/todo/entities/todo.py around lines 52-53, the timestamps are set
with datetime.now() which yields local time; change these to use UTC by calling
datetime.now(timezone.utc) (ensure timezone is imported from datetime), and
propagate this UTC change to all other noted spots: replace datetime.now() with
datetime.now(timezone.utc) at the default-parameter lines (33-34), the update
methods (lines 104, 113, 118, 130), and the is_overdue comparison (line 152);
also update tests in tests/domain/todo/entities/test_todo.py (lines 36-37) to
expect UTC timestamps or construct expected datetimes using timezone.utc.

self._completed_at = completed_at

def __eq__(self, obj: object) -> bool:
Expand Down