filter_lines: drop tzinfo before comparing parsed timestamps to --start-time/--end-time - #29
Open
HrachShah wants to merge 2 commits into
Open
filter_lines: drop tzinfo before comparing parsed timestamps to --start-time/--end-time#29HrachShah wants to merge 2 commits into
HrachShah wants to merge 2 commits into
Conversation
added 2 commits
July 4, 2026 17:31
…start-time/--end-time comparable
…rt-time/--end-time
The previous compare-tz-aware-to-naive path raised TypeError on
`<=>/<` between a tz-aware log timestamp and a naive CLI bound
("Cannot compare tz-naive and tz-aware datetime-like objects"),
so any line with an offset (e.g. Apache/JSON logs with +00:00)
silently disappeared from filtered output. cli._parse_file already
handles this with `timestamp.replace(tzinfo=None)` when a tz is
attached; mirror that here in the underlying filter_lines helper
so the helper, the CLI, and any direct library caller all agree on
how a tz-aware log entry is compared against a tz-naive bound.
Two new TestFilterLinesTzAware cases pin the behaviour: a
15:00:00+00:00 entry is correctly dropped against a naive 16:00:00
start, and a 17:00:00+00:00 entry is kept.
Reviewer's GuideAligns timestamp parsing and filtering between the core utils and CLI by properly handling timezone-aware datetimes (including Apache-style and ISO 8601 with offsets), and normalizes tz-aware timestamps to naive for comparison against naive start/end bounds, with new tests to lock in parsing and filtering behavior. Sequence diagram for tz-aware timestamp filtering in CLI and utilssequenceDiagram
actor User
participant CLI as cli._parse_file
participant Utils as utils.filter_lines
participant Parser as parse_timestamp
User->>CLI: _parse_file(start_time, end_time)
loop for each line
CLI->>Parser: parse_timestamp(line)
Parser-->>CLI: timestamp
alt [start_time or end_time]
alt [timestamp and timestamp.tzinfo]
CLI->>CLI: timestamp.replace(tzinfo=None) as compare_ts
else [timestamp is naive or None]
CLI->>CLI: compare_ts = timestamp
end
CLI->>CLI: compare_ts < start_time?
CLI->>CLI: compare_ts > end_time?
end
end
User->>Utils: filter_lines(lines, start_time, end_time)
loop for each line
Utils->>Parser: parse_timestamp(line)
Parser-->>Utils: timestamp
alt [start_time]
alt [timestamp and timestamp.tzinfo]
Utils->>Utils: timestamp.replace(tzinfo=None) as compare_ts
else [timestamp is naive or None]
Utils->>Utils: compare_ts = timestamp
end
Utils->>Utils: skip if compare_ts < start_time
end
alt [end_time]
alt [timestamp and timestamp.tzinfo]
Utils->>Utils: timestamp.replace(tzinfo=None) as compare_ts
else [timestamp is naive or None]
Utils->>Utils: compare_ts = timestamp
end
Utils->>Utils: skip if compare_ts > end_time
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughTimestamp parsing in utils.py was extended to support optional timezone offsets and additional datetime formats. Filtering logic in filter_lines (utils.py) and _parse_file (cli.py) now normalizes tz-aware timestamps to tz-naive before comparing against start_time/end_time bounds. New tests validate parsing and filtering behavior. ChangesTimezone-aware timestamp handling
Estimated code review effort: 2 (Simple) | ~15 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The tz-normalization logic for timestamp comparisons is now duplicated between
filter_linesandcli._parse_file; consider extracting a small helper (e.g.,normalize_timestamp_for_bounds(timestamp)) to keep the behavior consistent and reduce repetition. - In
filter_lines,compare_tsis recomputed separately for the start and end checks; you could compute it once whentimestampand either bound are present to avoid redundant work and simplify the conditional flow. - The updated
_try_parse_datetimeformats andtimestamp_patternscomment on ordering and specificity are helpful; you might also add a brief note in the regex for ISO timestamps clarifying that bothZand numeric offsets are supported, mirroring the detail given for Apache common log.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The tz-normalization logic for timestamp comparisons is now duplicated between `filter_lines` and `cli._parse_file`; consider extracting a small helper (e.g., `normalize_timestamp_for_bounds(timestamp)`) to keep the behavior consistent and reduce repetition.
- In `filter_lines`, `compare_ts` is recomputed separately for the start and end checks; you could compute it once when `timestamp` and either bound are present to avoid redundant work and simplify the conditional flow.
- The updated `_try_parse_datetime` formats and `timestamp_patterns` comment on ordering and specificity are helpful; you might also add a brief note in the regex for ISO timestamps clarifying that both `Z` and numeric offsets are supported, mirroring the detail given for Apache common log.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![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 (2)
tests/test_utils.py (1)
144-166: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
end_timetest coverage for tz-aware entries.The suite validates
start_timefiltering with tz-aware entries but doesn't testend_time. The normalization code path forend_time(lines 220-227 inutils.py) is symmetric but untested.💚 Suggested additional test
def test_filter_lines_tz_aware_entry_after_naive_start(self) -> None: lines = ["2025-10-10T17:00:00+00:00 INFO kept"] start = datetime(2025, 10, 10, 16, 0, 0) results = list(filter_lines(iter(lines), start_time=start)) assert len(results) == 1 assert results[0][1] == "2025-10-10T17:00:00+00:00 INFO kept" + + def test_filter_lines_tz_aware_entry_after_naive_end(self) -> None: + """A tz-aware entry at 17:00:00+00:00 must be dropped when + compared against a naive end_time of 16:00:00.""" + lines = ["2025-10-10T17:00:00+00:00 INFO hello"] + end = datetime(2025, 10, 10, 16, 0, 0) + results = list(filter_lines(iter(lines), end_time=end)) + assert results == [] + + def test_filter_lines_tz_aware_entry_before_naive_end(self) -> None: + """A tz-aware entry at 15:00:00+00:00 must be kept when + compared against a naive end_time of 16:00:00.""" + lines = ["2025-10-10T15:00:00+00:00 INFO kept"] + end = datetime(2025, 10, 10, 16, 0, 0) + results = list(filter_lines(iter(lines), end_time=end)) + assert len(results) == 1 + assert results[0][1] == "2025-10-10T15:00:00+00:00 INFO kept"🤖 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 `@tests/test_utils.py` around lines 144 - 166, Add test coverage for timezone-aware entries against end_time in TestFilterLinesTzAware, mirroring the existing start_time cases so the same normalization path is exercised in filter_lines. Create one test where a tz-aware log entry before a naive end_time is kept and one where a tz-aware entry after the naive end_time is dropped, asserting the behavior matches the start_time handling and does not raise a timezone comparison error.src/log_analyzer_cli/utils.py (1)
208-227: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared tz-aware normalization helper to eliminate duplication.
The tz-aware-to-naive normalization logic is duplicated between
filter_lineshere and_parse_fileincli.py(lines 217-225). Extracting a small helper would keep the two paths consistent and prevent future drift.♻️ Proposed helper extraction
+def _normalize_to_naive(timestamp: Optional[datetime]) -> Optional[datetime]: + """Strip tzinfo from a tz-aware timestamp for naive bound comparison.""" + if timestamp and timestamp.tzinfo: + return timestamp.replace(tzinfo=None) + return timestamp + def filter_lines( lines: Generator[str, None, None], include_levels: Optional[list[str]] = None, exclude_levels: Optional[list[str]] = None, search_pattern: Optional[str] = None, start_time: Optional[datetime] = None, end_time: Optional[datetime] = None, ) -> Generator[tuple[int, str, Optional[datetime], str], None]: """Filter log lines based on various criteria. ... """ compiled_pattern = re.compile(search_pattern) if search_pattern else None for line_num, line in enumerate(lines, 1): line = line.rstrip("\n\r") if not line: continue level = detect_log_level(line) if include_levels and level not in include_levels: continue if exclude_levels and level in exclude_levels: continue if compiled_pattern and not compiled_pattern.search(line): continue timestamp = parse_timestamp(line) - if start_time and timestamp: - # Normalize a tz-aware timestamp to naive so a naive --start-time - # bound can be compared against log entries with a UTC offset, - # while preserving the existing behavior for tz-naive entries. - compare_ts = ( - timestamp.replace(tzinfo=None) - if timestamp.tzinfo - else timestamp - ) - if compare_ts < start_time: - continue - - if end_time and timestamp: - compare_ts = ( - timestamp.replace(tzinfo=None) - if timestamp.tzinfo - else timestamp - ) - if compare_ts > end_time: - continue + if start_time and timestamp: + compare_ts = _normalize_to_naive(timestamp) + if compare_ts < start_time: + continue + + if end_time and timestamp: + compare_ts = _normalize_to_naive(timestamp) + if compare_ts > end_time: + continue yield line_num, line, timestamp, levelThen in
cli.py:_parse_file, replace the inline normalization with the same helper:- compare_ts = timestamp.replace(tzinfo=None) if (timestamp and timestamp.tzinfo) else timestamp + compare_ts = _normalize_to_naive(timestamp)🤖 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 `@src/log_analyzer_cli/utils.py` around lines 208 - 227, The tz-aware-to-naive timestamp normalization is duplicated in filter_lines and _parse_file, so extract that shared logic into a small helper and use it in both places. Create a single normalization function near the existing timestamp handling in utils.py, then update the comparisons in filter_lines and the corresponding code in cli.py to call that helper so both paths stay consistent and avoid future drift.
🤖 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 `@tests/test_utils.py`:
- Around line 7-14: Remove the unused imports in the test_utils module to
satisfy flake8 F401. In the import block that pulls in pytest and symbols from
log_analyzer_cli.utils, delete pytest and read_log_file if they are not
referenced anywhere else in the file. Keep only the imports actually used by the
test helpers/functions in this module.
---
Nitpick comments:
In `@src/log_analyzer_cli/utils.py`:
- Around line 208-227: The tz-aware-to-naive timestamp normalization is
duplicated in filter_lines and _parse_file, so extract that shared logic into a
small helper and use it in both places. Create a single normalization function
near the existing timestamp handling in utils.py, then update the comparisons in
filter_lines and the corresponding code in cli.py to call that helper so both
paths stay consistent and avoid future drift.
In `@tests/test_utils.py`:
- Around line 144-166: Add test coverage for timezone-aware entries against
end_time in TestFilterLinesTzAware, mirroring the existing start_time cases so
the same normalization path is exercised in filter_lines. Create one test where
a tz-aware log entry before a naive end_time is kept and one where a tz-aware
entry after the naive end_time is dropped, asserting the behavior matches the
start_time handling and does not raise a timezone comparison error.
🪄 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: 74a9462a-01e3-4e6a-8399-8d6d2cda89df
📒 Files selected for processing (3)
src/log_analyzer_cli/cli.pysrc/log_analyzer_cli/utils.pytests/test_utils.py
Comment on lines
+7
to
+14
| import pytest | ||
|
|
||
| from log_analyzer_cli.utils import ( | ||
| detect_log_level, | ||
| filter_lines, | ||
| normalize_error_pattern, | ||
| parse_timestamp, | ||
| read_log_file, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔴 Critical | ⚡ Quick win
Remove unused imports causing CI failures.
pytest and read_log_file are imported but never used. flake8 F401 is failing CI on both Python 3.10 and 3.11.
🛡️ Proposed fix
from datetime import datetime, timezone, timedelta
-import pytest
-
from log_analyzer_cli.utils import (
detect_log_level,
filter_lines,
normalize_error_pattern,
parse_timestamp,
- read_log_file,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import pytest | |
| from log_analyzer_cli.utils import ( | |
| detect_log_level, | |
| filter_lines, | |
| normalize_error_pattern, | |
| parse_timestamp, | |
| read_log_file, | |
| import pytest | |
| from log_analyzer_cli.utils import ( | |
| detect_log_level, | |
| filter_lines, | |
| normalize_error_pattern, | |
| parse_timestamp, | |
| ) |
🧰 Tools
🪛 GitHub Actions: CI / 1_test (3.10).txt
[error] 7-7: F401 'pytest' imported but unused
[error] 9-9: F401 'log_analyzer_cli.utils.read_log_file' imported but unused
🪛 GitHub Actions: CI / 3_test (3.11).txt
[error] 7-7: flake8 (F401) 'pytest' imported but unused
[error] 9-9: flake8 (F401) 'log_analyzer_cli.utils.read_log_file' imported but unused
🪛 GitHub Actions: CI / test (3.10)
[error] 7-7: flake8: F401 'pytest' imported but unused
[error] 9-9: flake8: F401 'log_analyzer_cli.utils.read_log_file' imported but unused
🪛 GitHub Actions: CI / test (3.11)
[error] 7-7: flake8 F401 'pytest' imported but unused
[error] 9-9: flake8 F401 'log_analyzer_cli.utils.read_log_file' imported but unused
🤖 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 `@tests/test_utils.py` around lines 7 - 14, Remove the unused imports in the
test_utils module to satisfy flake8 F401. In the import block that pulls in
pytest and symbols from log_analyzer_cli.utils, delete pytest and read_log_file
if they are not referenced anywhere else in the file. Keep only the imports
actually used by the test helpers/functions in this module.
Source: Pipeline failures
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The previous compare-tz-aware-to-naive path raised
TypeErroron<=>/<between a tz-aware log timestamp and a naive CLI bound, so any line with an offset (e.g. Apache/JSON logs with+00:00) silently disappeared from filtered output.cli._parse_filealready handles this withtimestamp.replace(tzinfo=None)when a tz is attached; mirror that here in the underlyingfilter_lineshelper so the helper, the CLI, and any direct library caller all agree on how a tz-aware log entry is compared against a tz-naive bound.Two new
TestFilterLinesTzAwarecases pin the behaviour: a15:00:00+00:00entry is correctly dropped against a naive16:00:00start, and a17:00:00+00:00entry is kept. Full suite: 75 passed (was 73 + 2 new).Summary by Sourcery
Improve timestamp parsing and time-bound filtering to correctly handle timezone-aware log entries against naive start/end times.
New Features:
Bug Fixes:
Tests:
Summary by CodeRabbit
Bug Fixes
Tests