Add type hints to log aggregator#14
Conversation
|
Warning Review limit reached
More reviews will be available in 2 minutes and 56 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthrough
ChangesType Hints and Diagnostic Artifact
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/log_aggregator.py (1)
365-376:⚠️ Potential issue | 🟡 MinorType narrowing won't satisfy strict type checkers.
The filter
e.get('timestamp') is not Nonedoesn't narrow the type ofe['timestamp']in the list comprehension. Type checkers infertimestampsaslist[Optional[Union[int, float]]], somin()andmax()would returnOptional[Union[int, float]], not the non-None type expected bydatetime.fromtimestamp(). This causes type errors in strict checking modes.🔧 Suggested fix
def _get_time_range(self) -> Optional[TimeRange]: - timestamps = [ - e['timestamp'] for e in self.entries - if e.get('timestamp') is not None + timestamps: list[Union[int, float]] = [ + ts for e in self.entries + if (ts := e.get('timestamp')) is not None ]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/log_aggregator.py` around lines 365 - 376, The type filter in the _get_time_range method's list comprehension does not properly narrow the type for strict type checkers, leaving timestamps as Optional values that cannot be safely passed to min() and max(). Restructure the list comprehension to explicitly cast or narrow the filtered timestamp values to their non-None types, or use typing.cast() to explicitly indicate that the filtered values are guaranteed to be non-None numeric types (int or float). This will ensure that the result of min(timestamps) and max(timestamps) is properly typed as a non-optional numeric value that datetime.fromtimestamp() can accept without type errors.
🧹 Nitpick comments (1)
tools/log_aggregator.py (1)
136-148: 💤 Low valueConsider using tuples for immutable class-level patterns.
Ruff RUF012 flags these as mutable class attribute defaults. While mutation is unlikely here, converting to tuples prevents accidental modification and satisfies the linter.
♻️ Suggested fix
- TIMESTAMP_PATTERNS: list[tuple[str, str]] = [ + TIMESTAMP_PATTERNS: tuple[tuple[str, str], ...] = ( (r'^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}', 'iso8601'), (r'^\d{4}-\d{2}-\d{2} \d{2}:\d{2}:\d{2}', 'standard'), (r'^\[?\d{2}/\w{3}/\d{4}:\d{2}:\d{2}:\d{2}', 'nginx'), (r'^\w{3}\s+\d{1,2}\s+\d{2}:\d{2}:\d{2}', 'syslog'), - ] + ) - LEVEL_PATTERNS: list[tuple[str, str]] = [ + LEVEL_PATTERNS: tuple[tuple[str, str], ...] = ( (r'\b(ERROR|FATAL|CRITICAL)\b', 'error'), (r'\b(WARN|WARNING)\b', 'warn'), (r'\b(INFO|NOTICE)\b', 'info'), (r'\b(DEBUG|TRACE)\b', 'debug'), - ] + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/log_aggregator.py` around lines 136 - 148, Convert the TIMESTAMP_PATTERNS and LEVEL_PATTERNS class attributes from lists to tuples to make them immutable and satisfy the Ruff RUF012 linter warning about mutable class attribute defaults. Replace the square brackets that wrap the tuple elements with parentheses for both TIMESTAMP_PATTERNS and LEVEL_PATTERNS, leaving the individual tuples inside unchanged.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@diagnostic/build-63a9dfe9.json`:
- Around line 9-10: Replace the redacted placeholder values in the `password`
and `decrypt_command` fields with the actual concrete diagnostic credentials.
The build.py script and the workflow require these real values to properly
unpack the referenced .logd diagnostic artifact. Update both fields to contain
the actual password and decrypt command values instead of the
`<redacted-public-pr>` placeholders.
In `@tools/log_aggregator.py`:
- Line 536: The f-string prefix on the print statement for "\nSummary:" is
unnecessary because the string contains no interpolation expressions or
placeholders. Remove the f prefix from the string literal in the print function
call to use a regular string instead, making it print("\nSummary:") without the
f prefix.
---
Outside diff comments:
In `@tools/log_aggregator.py`:
- Around line 365-376: The type filter in the _get_time_range method's list
comprehension does not properly narrow the type for strict type checkers,
leaving timestamps as Optional values that cannot be safely passed to min() and
max(). Restructure the list comprehension to explicitly cast or narrow the
filtered timestamp values to their non-None types, or use typing.cast() to
explicitly indicate that the filtered values are guaranteed to be non-None
numeric types (int or float). This will ensure that the result of
min(timestamps) and max(timestamps) is properly typed as a non-optional numeric
value that datetime.fromtimestamp() can accept without type errors.
---
Nitpick comments:
In `@tools/log_aggregator.py`:
- Around line 136-148: Convert the TIMESTAMP_PATTERNS and LEVEL_PATTERNS class
attributes from lists to tuples to make them immutable and satisfy the Ruff
RUF012 linter warning about mutable class attribute defaults. Replace the square
brackets that wrap the tuple elements with parentheses for both
TIMESTAMP_PATTERNS and LEVEL_PATTERNS, leaving the individual tuples inside
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6389f0fd-30ac-4a20-a966-106cfefde851
📒 Files selected for processing (3)
diagnostic/build-63a9dfe9.jsondiagnostic/build-63a9dfe9.logdtools/log_aggregator.py
1 participant
Summary
Adds complete, specific type annotations to the public parser and aggregation APIs in
tools/log_aggregator.py. Fixes #1.Changes
Any.LogAggregatorclass docstring and kept parser class docstrings in place.Testing
python3 -m py_compile tools/log_aggregator.pypython3 tools/log_aggregator.py --helppython3 -m json.toolon the generated reportpython3 -m json.toolon the generated reportpython3 build.pydiagnostic/build-63a9dfe9.logd,diagnostic/build-63a9dfe9.jsonfrailboxfails on unavailableMAP_HUGETLB;openapi-haskellfails becauseData.Aeson/Data.Yamlpackages are unavailable in the local GHC environmentChecklist
Summary by CodeRabbit
Refactor
Chores