chore: update version to 1.4.15 and add timezone support in date hand… #40
Conversation
WalkthroughThis pull request involves updating the package version number in Changes
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tc_analyzer_lib/metrics/analyzer_memberactivities.py (1)
76-78: Verify MongoDB date handling with timezone-aware dates.The addition of UTC timezone to datetime objects may affect MongoDB queries and data storage. Ensure that:
- MongoDB correctly handles timezone-aware datetime objects
- Existing data in the database is compatible with the new timezone-aware queries
- Date comparisons in MongoDB queries remain accurate
Consider documenting the timezone handling approach in the codebase to maintain consistency in future changes.
Also applies to: 92-92
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
setup.py(1 hunks)tc_analyzer_lib/metrics/analyzer_memberactivities.py(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- setup.py
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: ci / lint / Lint
- GitHub Check: ci / test / Test
🔇 Additional comments (3)
tc_analyzer_lib/metrics/analyzer_memberactivities.py (3)
2-2: LGTM! Necessary import for timezone support.The addition of
timezoneimport is appropriate for implementing UTC timezone awareness.
76-78: Verify timezone handling in dependent code.The UTC timezone addition is correct. However, we should verify that any code comparing dates with this 'today' variable is timezone-aware to prevent unexpected behavior.
✅ Verification successful
The timezone addition is safe and locally scoped.
The UTC timezone addition to
todayvariable is used only for date comparisons within the method, and all other datetime.now() usages in the codebase are for relative time calculations (timedelta) or date-only comparisons, which are timezone-independent.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for date comparisons with 'today' variable rg -l "today\s*[<>=]" | xargs rg -A 2 -B 2 "today\s*[<>=]" # Search for other datetime.now() usage that might need similar timezone handling ast-grep --pattern 'datetime.now()'Length of output: 54226
92-92: Verify date range calculations with timezone-aware dates.The UTC timezone addition to
first_datealigns with thetodayvariable change. However, we should verify that all date range calculations (especially withtimedelta) handle timezone-aware dates correctly.✅ Verification successful
Timezone-aware date calculations verified as correct.
The UTC timezone addition to
first_dateis safe and properly aligns with the codebase's date handling patterns. Python's timedelta operations preserve timezone information and work correctly with timezone-aware dates.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for date range calculations and timedelta usage rg -A 3 -B 3 "timedelta\(days=" # Search for other datetime replacements that might need timezone ast-grep --pattern $'$_.replace(\n $$$\n)'Length of output: 112419
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tc_analyzer_lib/tc_analyzer.py (1)
193-193: Consider refactoring to reduce code duplicationThe timezone handling is correctly implemented, but there's duplicate code between
run_onceandrecomputemethods, as noted by the TODO comment. Consider extracting the common MemberActivities initialization into a helper method.Here's a suggested refactor:
def _init_member_activities(self) -> MemberActivities: """Initialize MemberActivities with common configuration.""" return MemberActivities( platform_id=self.platform_id, resources=self.resources, action_config=self.action, window_config=self.window, analyzer_config=self.analyzer_config, analyzer_period=self.period.replace(tzinfo=timezone.utc), )This would simplify both methods and ensure consistent initialization.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tc_analyzer_lib/tc_analyzer.py(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ci / test / Test
🔇 Additional comments (1)
tc_analyzer_lib/tc_analyzer.py (1)
2-2: LGTM: Import addition for timezone supportThe addition of the timezone import is appropriate for the new UTC timezone handling functionality.
| @@ -105,7 +105,7 @@ async def run_once(self): | |||
| action_config=self.action, | |||
| window_config=self.window, | |||
| analyzer_config=self.analyzer_config, | |||
| analyzer_period=self.period, | |||
| analyzer_period=self.period.replace(tzinfo=timezone.utc), | |||
There was a problem hiding this comment.
💡 Codebase verification
Timezone handling needs to be consistent
The verification confirms that period can be timezone-naive when passed to the constructor. For example, in setup_platform.py, the period is created without timezone information. Adding timezone.utc in run_once is correct, but we should also ensure timezone awareness in the constructor to maintain consistency.
- Consider adding timezone validation in
TCAnalyzer.__init__to ensureperiodis always timezone-aware - Update test utilities like
setup_platform.pyto create timezone-aware datetime objects
🔗 Analysis chain
Verify timezone handling in constructor
While adding UTC timezone here is correct, we should ensure that self.period is consistently handled when it's initially set in the constructor.
Let's verify the timezone handling in the constructor and any potential callers:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for TCAnalyzer instantiations to verify period parameter handling
rg -l "TCAnalyzer\(" | xargs rg -B 5 -A 1 "TCAnalyzer\("
# Search for period parameter assignments in constructor
ast-grep --pattern 'class TCAnalyzer {
$$$
__init__($$$) {
$$$
self.period = period
$$$
}
}'
Length of output: 1543
Script:
#!/bin/bash
# Get the complete constructor implementation
ast-grep --pattern 'class TCAnalyzer {
$$$
def __init__($$$) {
$$$
}
}'
# Find period parameter usage in setup_platform.py
rg -B 5 -A 5 "TCAnalyzer\(" tests/integration/utils/setup_platform.py
# Look for period type hints or related imports
rg "from datetime" tc_analyzer_lib/tc_analyzer.py
Length of output: 506
Script:
#!/bin/bash
# Try different pattern for constructor
ast-grep --pattern 'class TCAnalyzer($_) {
def __init__(self, $$$) {
$$$
}
}'
# Find period variable definition in setup_platform
rg -B 10 "period=" tests/integration/utils/setup_platform.py
# Look for period-related type hints
rg "period.*:" tc_analyzer_lib/tc_analyzer.py
Length of output: 579
Script:
#!/bin/bash
# Find period variable definition with more context
rg -B 15 -A 5 "days_ago_period" tests/integration/utils/setup_platform.py
# Try one more pattern for constructor
ast-grep --pattern 'class TCAnalyzer {
def __init__(self, platform_id: str, period: datetime, $$$) {
$$$
}
}'
Length of output: 2328
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
tc_analyzer_lib/models/MemberActivityModel.py (1)
Line range hint
16-28: Ensure timezone consistency in returned datetime.Since MongoDB stores dates in UTC by default and the PR aims to add timezone support, the returned date should be explicitly set to UTC timezone to maintain consistency with other changes in the codebase.
Apply this diff:
def get_last_date(self) -> datetime | None: """ Gets the date of the last document """ try: date = ( self.database[self.collection_name] .find() .sort([("date", pymongo.DESCENDING)]) .limit(1)[0]["date"] ) - return date + return date.replace(tzinfo=timezone.utc) if date else None except Exception as e: print(e) return None
🧹 Nitpick comments (2)
tc_analyzer_lib/models/MemberActivityModel.py (1)
Line range hint
16-28: Improve error handling and documentation.
- Use the logger instead of print for error handling
- Add return type information to docstring
Apply these improvements:
def get_last_date(self) -> datetime | None: """ Gets the date of the last document + + Returns: + -------- + datetime | None + The date of the last document in UTC timezone if exists, None otherwise """ try: date = ( self.database[self.collection_name] .find() .sort([("date", pymongo.DESCENDING)]) .limit(1)[0]["date"] ) return date.replace(tzinfo=timezone.utc) if date else None except Exception as e: - print(e) + logging.error(f"Failed to get last date: {e}") return Nonetc_analyzer_lib/algorithms/member_activity_history.py (1)
Line range hint
10-40: Consider refactoring for better maintainability.The function is quite long and handles multiple responsibilities. Consider these improvements:
- Split the function into smaller, focused functions
- Use more descriptive variable names (e.g.,
date_range→analysis_period)- Improve type hints using more specific types
Example of improved type hints:
-def check_past_history( - db_access: DB_access, - date_range: list[datetime], - window_param: dict[str, int], +from typing import TypedDict, Tuple, Dict, Optional + +class WindowParams(TypedDict): + period_size: int + step_size: int + +def check_past_history( + db_access: DB_access, + date_range: Tuple[datetime, datetime], + window_param: WindowParams,Would you like me to provide a complete refactoring suggestion that splits this function into smaller, more focused functions?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tc_analyzer_lib/algorithms/member_activity_history.py(2 hunks)tc_analyzer_lib/metrics/analyzer_memberactivities.py(4 hunks)tc_analyzer_lib/models/MemberActivityModel.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tc_analyzer_lib/metrics/analyzer_memberactivities.py
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: ci / lint / Lint
- GitHub Check: ci / test / Test
| if db_analysis_end_date: | ||
| db_analysis_end_date = db_analysis_end_date.replace(tzinfo=timezone.utc) |
There was a problem hiding this comment.
Ensure consistent timezone handling for all datetime objects.
While db_analysis_end_date is now timezone-aware, the comparisons with date_range_start and date_range_end could raise TypeError if they are naive datetime objects.
Apply this fix to ensure all datetime comparisons are timezone-aware:
if db_analysis_end_date:
db_analysis_end_date = db_analysis_end_date.replace(tzinfo=timezone.utc)
+
+ # Ensure date range is timezone-aware
+ date_range_start = date_range_start.replace(tzinfo=timezone.utc) if date_range_start.tzinfo is None else date_range_start
+ date_range_end = date_range_end.replace(tzinfo=timezone.utc) if date_range_end.tzinfo is None else date_range_endCommittable suggestion skipped: line range outside the PR's diff.
…ling
Summary by CodeRabbit
Chores
Improvements