Skip to content

Conversation

@hshang315
Copy link
Collaborator

@hshang315 hshang315 commented Jan 16, 2026

Fixes issue #75

  • Add TIME_PARSING_LOCAL environment variable to enable local timezone
  • Add TZ environment variable support for configurable timezone
  • Update _get_time_parsing_system_prompt() to accept local parameter
  • Change validation from "future years" to "future dates" check
  • Allow past years (2025, 2024, etc.) when before current time
  • Add documentation for the new feature

This allows applications like control systems to use local timezone (e.g., America/Chicago) instead of UTC when operators work in local time.

Description

Related Issue

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature causing existing functionality to break)
  • Documentation update
  • Infrastructure/tooling change
  • Refactoring (no functional changes)
  • Style/formatting changes
  • Performance improvement
  • Test updates

Changes Made

Breaking Changes

None / See details below:

Testing

Test Environment

  • Local development
  • CI/CD pipeline
  • Manual testing
  • Automated tests added/updated

Test Cases

Documentation

  • Code changes are self-documenting
  • Docstrings updated (if applicable)
  • RST documentation updated (if applicable)
  • README updated (if applicable)
  • Migration guide updated (for breaking changes)
  • CHANGELOG.md updated
  • No documentation needed

Checklist

  • Code follows project style guidelines (ruff)
  • Self-review completed
  • Comments added for complex/non-obvious code
  • Documentation updated (or N/A)
  • Tests added/updated (or N/A)
  • All tests passing locally
  • No new warnings introduced
  • Commits are clean and well-described
  • Branch is up-to-date with target branch

Framework-Specific Checks

  • Registry system changes validated
  • Configuration system compatibility verified
  • CLI commands tested
  • Templates validated (if modified)
  • Backward compatibility maintained (or breaking change documented)
  • No circular imports introduced

Screenshots / Output Examples

Before:

<!-- Paste relevant before state -->

After:

<!-- Paste relevant after state -->

Deployment Notes

  • Database migrations required
  • Configuration changes required
  • Environment variables added/changed
  • Dependencies added/updated
  • Service restart required

Reviewer Notes

Additional Context


@hshang315 hshang315 requested a review from thellert January 16, 2026 15:46
@hshang315 hshang315 force-pushed the feature/local-timezone-time-parsing branch from 0501b98 to 08eedeb Compare January 16, 2026 15:49
@thellert
Copy link
Collaborator

Thanks will look into that shortly! Don't bother about the failing CI tests I am currently fixing some environment/dependency issues that causes tests to fail on GH workflows

@hshang315
Copy link
Collaborator Author

Thanks for reviewing, I tested with ITS, it works fine with local time option. The purpose to add local time option is that I do not want to break other applications.

@hshang315 hshang315 force-pushed the feature/local-timezone-time-parsing branch from bf433a3 to 7563e68 Compare January 16, 2026 18:24
@thellert
Copy link
Collaborator

I just noticed that there are 14 changed files in this commit, and many of those are independent from the time zone. Was this by accident? please create a clean PR with only this feature/fix

@hshang315
Copy link
Collaborator Author

hshang315 commented Jan 16, 2026 via email

@hshang315
Copy link
Collaborator Author

please do not merge my approve my pull request, I will ask Marty to include my changes of time_range_parsing. thanks

@msmith0920 msmith0920 force-pushed the feature/local-timezone-time-parsing branch from 7563e68 to 4e6128d Compare January 18, 2026 13:50
@thellert
Copy link
Collaborator

Code Review by Claude Code

Based on a comprehensive review of this PR, here is my assessment:


Summary

This PR attempts to add local timezone support so operators can specify times in their local timezone instead of UTC. I recommend NOT merging in its current state due to several significant issues.


Critical Issues

1. No Tests Included

The PR modifies timezone handling logic (notoriously bug-prone) but includes zero tests. Timezone and DST edge cases are difficult to get right and require thorough test coverage.

2. Incorrect DST Handling (time_range_parsing.py:693-714)

start_local = datetime(
    start_naive.year, start_naive.month, start_naive.day,
    start_naive.hour, start_naive.minute, start_naive.second,
    start_naive.microsecond,
    tzinfo=local_tz,  # ← This is WRONG
)

Using tzinfo= directly doesn't properly handle DST transitions. For ZoneInfo, you should let the library determine DST, not force-assign the timezone. This will produce incorrect UTC conversions during DST transitions (spring forward/fall back).

3. DST Tolerance is a Code Smell (time_range_parsing.py:730-747)

DST_TOLERANCE_SECONDS = 3600  # 1 hour tolerance for DST transitions

Adding a 1-hour tolerance to mask incorrect DST handling is a workaround, not a fix. This hides bugs rather than solving them and could allow genuinely invalid time ranges to pass validation.

4. Duplicate Environment Variable Reads

TIME_PARSING_LOCAL is read twice: once in _get_timezone_info() and again in execute(). This is inefficient and risks inconsistency if the env var changes mid-execution.

5. PEP 8 Violation

TIME_PARSING_LOCAL = os.environ.get(...)  # Should be time_parsing_local

Python variable names should use snake_case, not SCREAMING_SNAKE_CASE (which is reserved for constants).

6. Dead Code: Python < 3.9 Fallback (time_range_parsing.py:385-389)

except ImportError:
    # Fallback for Python < 3.9

Per project guidelines, this codebase requires Python 3.11+. This fallback is dead code that adds maintenance burden.

7. PR Description Inconsistency

The PR description mentions "Add TZ environment variable support" but the code reads from system.timezone in config.yml, not a TZ environment variable. The documentation doesn't match the implementation.

8. Missing CHANGELOG Update

Per project guidelines: "Always keep the changelog in sync with every commit." No CHANGELOG entry was added.

9. Unchecked PR Template

All checkboxes in the PR template are unchecked, indicating no self-review, no testing, no documentation review was performed.


Minor Issues

  • Imports inside functions (ZoneInfo at line 365) instead of module level
  • No error handling for invalid timezone names in config
  • Inconsistent configuration approach: env var for toggle, config file for timezone name

What Should Be Done

  1. Add comprehensive tests covering:

    • Normal timezone conversion
    • DST spring-forward transitions
    • DST fall-back transitions
    • Invalid timezone configuration
    • Edge cases at year boundaries
  2. Fix DST handling by using proper timezone-aware datetime construction

  3. Remove the DST tolerance hack once DST is handled correctly

  4. Consolidate configuration - either use env vars or config file, not both

  5. Update CHANGELOG and documentation

  6. Remove dead code (Python < 3.9 fallback)

  7. Fix variable naming to follow PEP 8


Verdict

Do not merge. The core functionality (timezone handling) is implemented incorrectly and could produce wrong results during DST transitions. The lack of tests for such critical date/time logic is concerning. Please address these issues before re-review.

@hshang315
Copy link
Collaborator Author

hshang315 commented Jan 18, 2026 via email

Marty Smith added 2 commits January 19, 2026 07:50
- Enhanced time_range_parsing capability to use user's local timezone from config
- Added timezone configuration to ConfigurationSystem documentation
- Updated tests to verify timezone-aware parsing behavior
- Updated CHANGELOG with new timezone feature
@msmith0920 msmith0920 force-pushed the feature/local-timezone-time-parsing branch from 4e6128d to 18d79e3 Compare January 19, 2026 14:01
Marty Smith added 3 commits January 19, 2026 08:06
- Remove trailing whitespace from blank lines in time_range_parsing.py
- Remove unused imports (timezone, MagicMock) from test file
- Ensure imports are properly sorted per ruff I001 rule
- Fix I001 import sorting issue using ruff --fix
- All 81 tests still passing
- Auto-format file using ruff format
- All 81 tests still passing
@hshang315
Copy link
Collaborator Author

Please close this request, we will not submit our local time range parsing to osprey framework, instead, we will use it for our own appplications. It based on the osprey-framework with minor changes, just changed the UTC to local time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants