Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The changes are small, targeted fixes aligned with the issue/description, with only an optional logging-style improvement noted.
Pull request overview
This PR addresses Issue #162 by fixing Python logging calls that were dropping/triggering formatting errors due to missing %s placeholders, and by correcting a subtle test-data bug caused by implicit string literal concatenation in tests/test_ship.py.
Changes:
- Fix
logger.info()argument formatting inget_dist()(isochrone.py) so dist/time/bs values are actually emitted. - Fix
logger.info()formatting indetermine_timespread()(isofuel.py) sodelta_timeis included in logs. - Add missing commas in
tests/test_ship.pymessage arrays to prevent'Error' 'OK'from becoming'ErrorOK'.
File summaries
| File | Description |
|---|---|
| WeatherRoutingTool/algorithms/isofuel.py | Corrects logger.info() formatting for delta_time (and keeps time-spread logging adjacent). |
| WeatherRoutingTool/algorithms/isochrone.py | Corrects logger.info() formatting so computed distance/time inputs are logged correctly. |
| tests/test_ship.py | Fixes missing commas in test message arrays to prevent implicit string concatenation. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This branch has not been deployed
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
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.
Related Issue / Discussion
Fixes Issue #162.
Relates to open PRs #163 and #182 (see details below).
Changes
Please list the central functionalities that have been changed:
Modified
WeatherRoutingTool/algorithms/isochrone.pylogger.info()calls inget_dist()by adding%sformat specifiers so distance, speed, and time values are correctly included in log output instead of being silently dropped.Modified
WeatherRoutingTool/algorithms/isofuel.pylogger.info()call indetermine_timespread()to use%sformatting fordelta_time.Modified
tests/test_ship.pymessageNumPy array definitions withintest_shipparams_get_elementandtest_shipparams_get_single.'Error' 'OK'→'ErrorOK') and restores the expected four-element test arrays.Further Details
Summary
This PR resolves two small, mechanical bugs:
1. Logger
%sFormatting BugsIn
isochrone.pyandisofuel.py, somelogger.info()calls passed values as additional arguments without providing corresponding%splaceholders.For example:
and:
Python's standard logging module uses
%-style formatting for additional arguments. Without a corresponding format specifier, the supplied values are not included in the resulting log message.These calls have been corrected to:
and:
This ensures that the relevant distance, speed, time, and
delta_timevalues are properly displayed in the logs.2. Test Array Comma Typo
In
tests/test_ship.py, themessagearrays intest_shipparams_get_elementandtest_shipparams_get_singlecontained:without a separating comma.
Python implicitly concatenates adjacent string literals, resulting in:
'ErrorOK'instead of two separate elements.
This caused the array to contain only three elements instead of the expected four. Adding the missing comma restores the intended array:
and fixes the corresponding test input.
Note on Overlapping PRs #163 and #182
I am aware that open PRs #163 and #182 also address the
test_ship.pycomma typo. Both appear to be currently inactive.This PR combines that existing test-data fix with the logger formatting corrections related to Issue #162.
If maintainers prefer to merge #163 or #182 for the test fix separately, I am happy to rebase or isolate the logger-related changes into an independent PR.
Dependencies
None (no new dependencies required).
Testing
The affected test suite was run to verify the changes. The test-data correction ensures that the existing ship parameter tests operate on the intended four-element input arrays.
PR Checklist
In the context of this PR, I: