-
Notifications
You must be signed in to change notification settings - Fork 77
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.
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
base: main
Are you sure you want to change the base?
Conversation
WalkthroughModified the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dddpy/domain/todo/entities/todo.py(2 hunks)
🔇 Additional comments (1)
dddpy/domain/todo/entities/todo.py (1)
33-34: Excellent fix for the default argument anti-pattern!The signature change correctly addresses the issue where
datetime.now()was evaluated once at import time and reused across all instances. Moving toOptional[datetime] = Nonewith runtime evaluation is the proper pattern.
| self._created_at = created_at or datetime.now() | ||
| self._updated_at = updated_at or datetime.now() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 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.pyRepository: 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:
- The
orpattern fix is sound - The implementation correctly ensures runtime evaluation - 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.pylines 42-44) explicitly converts to UTC withtz=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.
Set
datetime.now()defaults insideTodo.__init__only.Any function call that's used in a default argument will only be performed once, at definition time. The returned value will then be reused by all calls to the function, which can lead to unexpected behaviour (See https://docs.astral.sh/ruff/rules/function-call-in-default-argument/)
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.