Implement boolean handling #39
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThe changes in the Changes
Assessment against linked issues
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
pyrail/models.py (1)
Add unit tests for boolean deserialization logic
The codebase lacks test coverage for the
str_to_boolfunction and boolean field deserialization. This is critical functionality used across multiple data models for train status, platform information, and cancellation flags.Add tests to
tests/test_irail.pycovering:
- Basic cases:
str_to_bool("1")returnsTruestr_to_bool("0")returnsFalse- Field deserialization:
canceledfield inLiveboardDeparturenormalfield inPlatformInfoleftandarrivedfields inVehicleStop- Error cases:
- Invalid string values
- Missing/null values
🔗 Analysis chain
Line range hint
1-516: Add unit tests for boolean deserialization.While the implementation looks correct, it would be beneficial to add unit tests that verify the boolean deserialization logic, especially for edge cases. This would help ensure that the fix for train cancellation status remains robust.
Consider testing:
- Valid cases ("0"/"1")
- Edge cases (invalid strings, empty values)
- Real data scenarios (cancelled/non-cancelled trains)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are any existing tests for boolean deserialization rg -l "test.*bool" "test_*.py"Length of output: 82
Script:
#!/bin/bash # Find all Python test files fd -e py "test_" # Search for boolean deserialization tests in Python files rg -l "str_to_bool|deserialize.*bool"Length of output: 93
🧹 Nitpick comments (1)
pyrail/models.py (1)
Line range hint
17-19: Consider making the boolean conversion more robust.The current implementation only handles "0"/"1" string values. Consider handling additional common boolean representations (e.g., "true"/"false", "yes"/"no") to make the function more resilient.
def str_to_bool(strbool: str) -> bool: """Convert a string ("0" or "1") to a boolean.""" - return strbool == "1" + if isinstance(strbool, bool): + return strbool + normalized = str(strbool).lower().strip() + if normalized in ("1", "true", "yes", "y"): + return True + if normalized in ("0", "false", "no", "n"): + return False + raise ValueError(f"Cannot convert {strbool} to boolean")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pyrail/models.py(8 hunks)
🔇 Additional comments (2)
pyrail/models.py (2)
85-85: LGTM! Boolean deserialization added correctly.The implementation correctly adds boolean deserialization for the
normalfield.
107-111: LGTM! Boolean fields properly deserialized.The implementation correctly adds boolean deserialization for all boolean fields, with appropriate aliases where needed.
|
@coderabbitai you say it does not address the linked issue, is it possible the linked issue is wrong? Right now the values are just strings with a 0 or 1 in it which should match false or true respectively. |
|
Tip For best results, initiate chat on the files or code changes. @tjorim The PR actually addresses the issue correctly, just in a different (and better) way than initially suggested in issue #28. Instead of changing the type to
The implementation looks correct and should fix the issue with trains being incorrectly marked as cancelled. 🔗 Analysis chain🏁 Scripts executedThe following scripts were executed for the analysis: Script: Length of output: 12640 ✏️ Learnings added
|
|
@coderabbitai generate tests for the str_to_bool function and boolean field deserialization. |
|
Tip For best results, initiate chat on the files or code changes. @tjorim Here are the test cases for the def test_str_to_bool():
"""Test the str_to_bool function that converts string values to boolean."""
from pyrail.models import str_to_bool
# Test valid inputs
assert str_to_bool("1") is True, "String '1' should convert to True"
assert str_to_bool("0") is False, "String '0' should convert to False"
# Test invalid inputs (should return False for anything other than "1")
assert str_to_bool("") is False, "Empty string should convert to False"
assert str_to_bool("true") is False, "String 'true' should convert to False"
assert str_to_bool("false") is False, "String 'false' should convert to False"
assert str_to_bool("2") is False, "String '2' should convert to False"
def test_boolean_field_deserialization():
"""Test the deserialization of boolean fields in models."""
from pyrail.models import PlatformInfo, LiveboardDeparture
# Test PlatformInfo boolean field
platform = PlatformInfo.from_dict({
"name": "1",
"normal": "1"
})
assert platform.normal is True, "Platform normal field should be True when '1'"
platform = PlatformInfo.from_dict({
"name": "1",
"normal": "0"
})
assert platform.normal is False, "Platform normal field should be False when '0'"
# Test LiveboardDeparture multiple boolean fields
departure = LiveboardDeparture.from_dict({
"id": "1",
"station": "Brussels",
"stationinfo": {
"@id": "1",
"id": "1",
"name": "Brussels",
"locationX": 4.3517,
"locationY": 50.8503,
"standardname": "Brussels-Central"
},
"time": "1705593600", # Example timestamp
"delay": 0,
"canceled": "1",
"left": "0",
"isExtra": "1",
"vehicle": "BE.NMBS.IC1234",
"vehicleinfo": {
"name": "IC1234",
"shortname": "IC1234",
"number": "1234",
"type": "IC",
"locationX": 4.3517,
"locationY": 50.8503,
"@id": "1"
},
"platform": "1",
"platforminfo": {
"name": "1",
"normal": "1"
},
"occupancy": {
"@id": "1",
"name": "LOW"
},
"departureConnection": "1"
})
# Verify boolean fields are correctly deserialized
assert departure.canceled is True, "Departure canceled field should be True when '1'"
assert departure.left is False, "Departure left field should be False when '0'"
assert departure.is_extra is True, "Departure is_extra field should be True when '1'"
assert departure.platform_info.normal is True, "Platform normal field should be True when '1'"These tests cover:
The tests verify that:
🔗 Analysis chain🏁 Scripts executedThe following scripts were executed for the analysis: Script: Length of output: 47282 ✏️ Learnings added
|
|
@coderabbitai generate tests for the timestamp_to_datetime function and timestamp field deserialization. |
|
Tip For best results, initiate chat on the files or code changes. @tjorim Here are the test cases for timestamp handling. Add these to from pyrail.models import (
timestamp_to_datetime,
ApiResponse,
Alert,
Disturbance,
LiveboardDeparture,
)
def test_timestamp_to_datetime():
"""Test the timestamp_to_datetime function."""
# Test valid timestamps
assert timestamp_to_datetime("1705593600") == datetime(2024, 1, 18, 16, 0) # 2024-01-18 16:00:00
assert timestamp_to_datetime("0") == datetime(1970, 1, 1, 0, 0) # Unix epoch
# Test error handling
with pytest.raises(ValueError):
timestamp_to_datetime("") # Empty string
with pytest.raises(ValueError):
timestamp_to_datetime("invalid") # Non-numeric string
with pytest.raises(ValueError):
timestamp_to_datetime("99999999999999999") # Timestamp too large
def test_timestamp_field_deserialization():
"""Test timestamp field deserialization in various models."""
# Test ApiResponse timestamp
api_response = ApiResponse.from_dict({
"version": "1.0",
"timestamp": "1705593600"
})
assert api_response.timestamp == datetime(2024, 1, 18, 16, 0)
# Test LiveboardDeparture time
departure = LiveboardDeparture.from_dict({
"id": "1",
"station": "Brussels",
"stationinfo": {
"@id": "1",
"id": "1",
"name": "Brussels",
"locationX": 4.3517,
"locationY": 50.8503,
"standardname": "Brussels-Central"
},
"time": "1705593600",
"delay": 0,
"canceled": "0",
"left": "0",
"isExtra": "0",
"vehicle": "BE.NMBS.IC1234",
"vehicleinfo": {
"name": "IC1234",
"shortname": "IC1234",
"number": "1234",
"type": "IC",
"locationX": 4.3517,
"locationY": 50.8503,
"@id": "1"
},
"platform": "1",
"platforminfo": {
"name": "1",
"normal": "1"
},
"departureConnection": "1"
})
assert departure.time == datetime(2024, 1, 18, 16, 0)
# Test Alert start_time and end_time
alert = Alert.from_dict({
"id": "1",
"header": "Test Alert",
"lead": "Test Lead",
"link": "https://example.com",
"startTime": "1705593600",
"endTime": "1705597200"
})
assert alert.start_time == datetime(2024, 1, 18, 16, 0)
assert alert.end_time == datetime(2024, 1, 18, 17, 0)
# Test Disturbance timestamp
disturbance = Disturbance.from_dict({
"title": "Test Disturbance",
"description": "Test Description",
"type": "disturbance",
"link": "https://example.com",
"timestamp": "1705593600",
"richtext": "<p>Test</p>",
"descriptionLinks": []
})
assert disturbance.timestamp == datetime(2024, 1, 18, 16, 0)These tests cover:
(｀･ω･´)ゞ 🔗 Analysis chain🏁 Scripts executedThe following scripts were executed for the analysis: Script: Length of output: 22315 ✏️ Learnings added
|
Fixes #28
The current published version handles all trains as cancelled.
Summary by CodeRabbit