Skip to content

Fix: Make ARRIVAL_TIME validation consistent and remove invalid sentinel default#146

Open
Agarwalchetan wants to merge 1 commit into52North:mainfrom
Agarwalchetan:bugfix/arrival-time-validation
Open

Fix: Make ARRIVAL_TIME validation consistent and remove invalid sentinel default#146
Agarwalchetan wants to merge 1 commit into52North:mainfrom
Agarwalchetan:bugfix/arrival-time-validation

Conversation

@Agarwalchetan
Copy link

Related Issue / Discussion:

Fixes Issue #145

Changes:

  • Modified WeatherRoutingTool/config.py

    • Changed ARRIVAL_TIME to Optional[datetime] = None
    • Updated datetime validation logic to handle None correctly
    • Corrected error messages to reference the validated field name
    • Replaced sentinel string comparisons with None checks in check_speed_determination
    • Added backward compatibility for legacy sentinel string ('9999-99-99T99:99Z')
  • Modified WeatherRoutingTool/tests/test_config.py

    • Added unit tests covering:

      • ARRIVAL_TIME omitted
      • Both ARRIVAL_TIME and BOAT_SPEED specified
      • Neither specified
      • Invalid datetime format validation

Further Details:

Summary:

Previously, ARRIVAL_TIME was typed as datetime but used the string sentinel '9999-99-99T99:99Z' as a default value. This string is not a valid datetime and required string comparisons in validation logic.

Additionally, the datetime validator emitted error messages that referenced only DEPARTURE_TIME, even when validating ARRIVAL_TIME.

This PR:

  • Replaces the sentinel string with None using Optional[datetime]
  • Updates validation logic to rely on semantic None checks
  • Corrects error messages to reflect the validated field
  • Preserves backward compatibility by treating the legacy sentinel string as an alias for “unset”
  • Adds unit tests to ensure consistent behavior

Behavioral outcome:

  • Validation succeeds when ARRIVAL_TIME is omitted
  • Validation fails if both ARRIVAL_TIME and BOAT_SPEED are specified
  • Validation fails if neither is specified
  • Error messages correctly reference the validated field

This improves type consistency, simplifies validation logic, and maintains existing functionality.


Dependencies:

No new dependencies introduced.


PR Checklist:

In the context of this PR, I:

Please consider that PRs which do not meet the requirements specified in the checklist will not be evaluated. Also, PRs with no activities will be closed after a reasonable amount of time.


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

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants