Replace pendulum with aniso8601 #146
Conversation
|
Warning Rate limit exceeded@jschlyter has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 18 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe pull request modifies the project's date and time handling by replacing the Changes
Sequence DiagramsequenceDiagram
participant Client
participant Helpers
participant AggregateFunction
Client->>Helpers: parse_iso8601_interval(interval)
Helpers-->>Client: Returns (start_time, duration)
Client->>AggregateFunction: Use parsed interval
AggregateFunction-->>Client: Process aggregate
Poem
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: 1
🧹 Nitpick comments (3)
tests/test_iso8601.py (1)
6-13: Enhance test coverage with additional test cases.While the current tests cover basic functionality, consider adding test cases for:
- Invalid interval formats to verify error handling
- Negative durations
- Different timezone formats (e.g., -0500, +0530)
- Microsecond precision
def test_parse_iso8601_interval(): # Existing tests... # Invalid format with pytest.raises(ValueError): parse_iso8601_interval("invalid") # Different timezone formats start, duration = parse_iso8601_interval("2025-01-15T08:56:58-05:00/PT1M") assert start == datetime(year=2025, month=1, day=15, hour=13, minute=56, second=58, tzinfo=timezone.utc) # Microsecond precision start, duration = parse_iso8601_interval("2025-01-15T08:56:58.123456+00:00/PT1M") assert start == datetime(year=2025, month=1, day=15, hour=8, minute=56, second=58, microsecond=123456, tzinfo=timezone.utc)aggrec/helpers.py (1)
117-119: Add docstring and improve type safety.The function would benefit from:
- A docstring explaining its purpose, parameters, and return values
- Type hints for better IDE support
- Validation of the returned values
def parse_iso8601_interval(interval: str) -> tuple[datetime, timedelta]: + """Parse an ISO 8601 time interval string into start time and duration. + + Args: + interval: An ISO 8601 time interval string (e.g., "2024-01-01T00:00:00Z/PT1H") + + Returns: + A tuple containing: + - start time as datetime (UTC) + - duration as timedelta + + Raises: + ValueError: If the interval string is invalid or cannot be parsed + """ t1, t2 = aniso8601.parse_interval(interval) + if not isinstance(t1, datetime) or not isinstance(t2, datetime): + raise ValueError("Invalid interval format") return t1, timedelta(seconds=(t2 - t1).total_seconds())aggrec/client.py (1)
51-51: Consider moving default_interval to a constant and ensure consistent ISO format.
- Move the default interval to a constant at module level
- Specify timespec in isoformat() for consistent output
+DEFAULT_AGGREGATE_INTERVAL_DURATION = "PT1M" + def main() -> None: - default_interval = f"{datetime.now(tz=timezone.utc).isoformat()}/PT1M" + default_interval = f"{datetime.now(tz=timezone.utc).isoformat(timespec='seconds')}/{DEFAULT_AGGREGATE_INTERVAL_DURATION}"Also applies to: 61-62
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
.github/workflows/test.yml(1 hunks)aggrec/aggregates.py(2 hunks)aggrec/client.py(3 hunks)aggrec/helpers.py(2 hunks)pyproject.toml(1 hunks)tests/test_iso8601.py(1 hunks)
🔇 Additional comments (3)
aggrec/aggregates.py (2)
287-294: LGTM! Well-structured error handling.The changes look good with:
- Proper error handling using try-except
- Clear error message with a helpful example
- Clean integration with the new parse_iso8601_interval function
Line range hint
1-1: Verify complete removal of pendulum.Let's verify that all pendulum imports and usages have been removed from the codebase.
✅ Verification successful
Pendulum has been completely removed from the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining pendulum imports or usages echo "Checking for pendulum imports..." rg "import pendulum" -l echo "Checking for pendulum usage..." rg "pendulum\." -l echo "Checking for pendulum in requirements..." rg "pendulum" pyproject.toml setup.py requirements.txtLength of output: 444
Script:
#!/bin/bash # Description: Broader check for pendulum references echo "Checking for 'from pendulum' imports..." rg "from pendulum" -l echo "Checking for 'as pendulum' imports..." rg "as pendulum" -l echo "Checking pyproject.toml content..." cat pyproject.toml echo "Checking for any file containing 'pendulum' string..." rg -i "pendulum" -lLength of output: 1938
pyproject.toml (1)
28-28: Verify dependency compatibility.
- The Python version in CI (3.13) conflicts with the project's requirement (^3.12). This needs to be aligned.
- Let's verify that no other dependencies require pendulum and that they're all compatible with aniso8601.
✅ Verification successful
Dependencies verified successfully
The migration from pendulum to aniso8601 is clean and properly implemented. No dependency conflicts or remaining pendulum usage found in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential dependency conflicts and pendulum requirements # Check if any dependency still requires pendulum poetry show --tree | grep -B2 -A2 pendulum # List all dependencies that might conflict with aniso8601 poetry show --tree | grep -B2 -A2 aniso8601 # Check if any Python files still import pendulum rg -l 'import\s+pendulum|from\s+pendulum'Length of output: 328
Script:
#!/bin/bash # Check for pendulum imports and usage rg -l "pendulum" # Check for aniso8601 imports and usage rg -l "aniso8601" # Look at pyproject.toml content cat pyproject.toml # Look for datetime handling patterns ast-grep --pattern 'from datetime import $$$' ast-grep --pattern 'import datetime'Length of output: 1969
Script:
#!/bin/bash # Check how aniso8601 is used in the codebase rg -A 5 "aniso8601" aggrec/helpers.py # Look for any ISO8601 related functions ast-grep --pattern 'def $_($$$) { $$$ aniso8601 $$$ }'Length of output: 385
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/test_iso8601.py (1)
8-32: LGTM! Comprehensive test coverage for ISO8601 interval parsing.The test cases effectively cover various scenarios including timestamp/duration, timestamp/timestamp, invalid formats, timezone handling, and microsecond precision.
Consider adding these additional test cases to enhance coverage:
- Duration with different units (hours, days, weeks)
- Edge cases (leap years, DST transitions)
- Negative durations
- Empty or null input
aggrec/helpers.py (1)
117-122: Enhance documentation and error handling.While the implementation is correct, consider these improvements:
- Add docstring examples of supported formats
- Add timezone normalization
- Enhance error handling for edge cases
Here's a suggested implementation:
def parse_iso8601_interval(interval: str) -> tuple[datetime, timedelta]: - """Parse ISO8601 interval and return resulting datetime and timedelta""" + """Parse ISO8601 interval and return resulting datetime and timedelta. + + Args: + interval: ISO8601 interval string (e.g., "2025-01-15T08:56:58+00:00/PT1M" + or "19840101T000000Z/19840115T000000Z") + + Returns: + Tuple of (start datetime in UTC, duration) + + Raises: + ValueError: If interval format is invalid or results in negative duration + """ t1, t2 = aniso8601.parse_interval(interval) if not isinstance(t1, datetime) or not isinstance(t2, datetime): raise ValueError("Invalid interval format") + # Ensure both datetimes are in UTC + if t1.tzinfo is None: + raise ValueError("Start time must include timezone") + if t2.tzinfo is None: + raise ValueError("End time must include timezone") + t1 = t1.astimezone(timezone.utc) + t2 = t2.astimezone(timezone.utc) + duration = timedelta(seconds=(t2 - t1).total_seconds()) + if duration.total_seconds() < 0: + raise ValueError("Duration cannot be negative") - return t1, timedelta(seconds=(t2 - t1).total_seconds()) + return t1, duration
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
aggrec/client.py(3 hunks)aggrec/helpers.py(2 hunks)tests/test_iso8601.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- aggrec/client.py
🔇 Additional comments (1)
aggrec/helpers.py (1)
3-5: Verify complete removal of pendulum dependency.Let's ensure all pendulum usages have been replaced across the codebase.
✅ Verification successful
✓ Pendulum dependency has been completely removed
The codebase has been thoroughly checked and shows no remaining traces of pendulum in imports, type hints, or dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any remaining pendulum imports or usages echo "Checking for pendulum imports..." rg "import pendulum|from pendulum" -l echo "Checking for pendulum type hints..." rg "pendulum\." -l echo "Checking if pendulum is still in dependencies..." if [ -f "pyproject.toml" ]; then rg "pendulum" pyproject.toml fiLength of output: 380
Script:
#!/bin/bash # Verify we can find Python files and detect imports echo "Checking Python files exist..." fd '\.py$' echo -e "\nVerifying import detection works..." # Check for datetime import which we know exists in helpers.py rg "from datetime import|import datetime" -lLength of output: 543
Summary by CodeRabbit
Release Notes
Dependencies
pendulumlibrary withaniso8601for improved date and time interval parsingNew Features
Tests