Add more data models for responses in pyrail #23
Merged
Conversation
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
📝 WalkthroughWalkthroughThe pull request modifies the Changes
Assessment against linked issues
Possibly related PRs
Poem
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (1)
pyrail/models.py (1)
23-26: Minor naming suggestion for clarity.
location_xandlocation_yare indeed descriptive, but consider usinglongitudeandlatitudeto avoid confusion and stay consistent with domain terminology.location_x: float = field(metadata=field_options(alias="locationX")) # Longitude of the station location_y: float = field(metadata=field_options(alias="locationY")) # Latitude of the station - standard_name: str = field(metadata=field_options(alias="standardname")) # ... + longitude: float = field(metadata=field_options(alias="locationX")) # ... + latitude: float = field(metadata=field_options(alias="locationY")) # ...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pyrail/models.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
pyrail/models.py
38-38: Missing docstring in public class
(D101)
44-44: Missing docstring in public class
(D101)
53-53: Missing docstring in public class
(D101)
62-62: Missing docstring in public class
(D101)
68-68: Missing docstring in public class
(D101)
75-75: Missing docstring in public class
(D101)
81-81: Missing docstring in public class
(D101)
89-89: Missing docstring in public class
(D101)
95-95: Missing docstring in public class
(D101)
100-100: Missing docstring in public class
(D101)
116-116: Missing docstring in public class
(D101)
121-121: Missing docstring in public class
(D101)
132-132: Missing docstring in public class
(D101)
🪛 GitHub Actions: Lint with Ruff
pyrail/models.py
[error] 38-38: Missing docstring in public class 'Departures'
[error] 44-44: Missing docstring in public class 'LiveboardResponse'
[error] 53-53: Missing docstring in public class 'VehicleInfo'
[error] 62-62: Missing docstring in public class 'Stops'
[error] 68-68: Missing docstring in public class 'VehicleResponse'
[error] 75-75: Missing docstring in public class 'SegmentComposition'
[error] 81-81: Missing docstring in public class 'Segment'
[error] 89-89: Missing docstring in public class 'CompositionSegments'
[error] 95-95: Missing docstring in public class 'CompositionResponse'
[error] 100-100: Missing docstring in public class 'Connection'
[error] 116-116: Missing docstring in public class 'ConnectionsResponse'
[error] 121-121: Missing docstring in public class 'Disturbance'
[error] 132-132: Missing docstring in public class 'DisturbancesResponse'
🔇 Additional comments (15)
pyrail/models.py (15)
2-2: No issues found in this blank line.
34-34: Looks good.
37-37: No issues found in this blank line.
43-43: No issues found in this blank line.
52-52: No issues found in this blank line.
60-60: No issues found in this blank line.
66-66: No issues found in this blank line.
73-73: No issues found in this blank line.
79-79: No issues found in this blank line.
87-87: No issues found in this blank line.
93-93: No issues found in this blank line.
98-98: No issues found in this blank line.
114-114: No issues found in this blank line.
119-119: No issues found in this blank line.
130-130: No issues found in this blank line.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
… update related classes
… with detailed attributes
… vehicle endpoints
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
tests/test_irail.py (1)
27-27: Consider moving the user agent string to a configuration file.The hardcoded user agent string contains personal contact information. Consider moving it to a configuration file for better maintainability and privacy.
- headers={"User-Agent": "pyRail (https://github.com/tjorim/pyrail; tielemans.jorim@gmail.com)"}, + headers={"User-Agent": USER_AGENT}, # Import from config.pypyrail/irail.py (1)
154-154: Move user agent string to configuration.For consistency with the test file and better maintainability, consider moving the user agent string to a shared configuration file.
pyrail/models.py (7)
19-37: Enhance docstring with type information.While the class is well-documented with inline comments, consider adding type information to the docstring for better IDE support.
@dataclass class StationDetails(DataClassORJSONMixin): - """Represents a single railway station with location and naming attributes.""" + """Represents a single railway station with location and naming attributes. + + Attributes: + at_id (str): The unique identifier of the station in the iRail system. + id (str): The (iRail) ID of the station. + name (str): Default name of the station. + longitude (float): Longitude coordinate of the station. + latitude (float): Latitude coordinate of the station. + standard_name (str): Consistent name of the station. + """
39-50: Add coordinate validation for vehicle location.Consider adding validation for longitude and latitude values to ensure they fall within valid ranges.
@dataclass class VehicleInfo(DataClassORJSONMixin): """Represents information about a specific vehicle, including name and location.""" name: str # Name of the vehicle short_name: str = field(metadata=field_options(alias="shortname")) # Shortened name of the vehicle number: str # Number of the vehicle type: str # Type of vehicle (e.g., IC, EC) - longitude: float = field(metadata=field_options(alias="locationX")) # Longitude of the vehicle - latitude: float = field(metadata=field_options(alias="locationY")) # Latitude of the vehicle + longitude: float = field(metadata=field_options(alias="locationX", validate=lambda x: -180 <= x <= 180)) + latitude: float = field(metadata=field_options(alias="locationY", validate=lambda x: -90 <= x <= 90)) at_id: str = field(metadata=field_options(alias="@id")) # ID of the vehicle
52-66: Use Literal type for platform normality status.Consider using a more strict type for the
normalfield to prevent invalid values.+from typing import Literal + @dataclass class PlatformInfo(DataClassORJSONMixin): """Details about the platform, such as name and whether it is the normal one.""" name: str # Platform name - normal: bool # Whether it is the normal platform + normal: Literal[0, 1] # Whether it is the normal platform (0: non-standard, 1: st 6293 andard)
68-105: Use enums for categorical fields.Consider using enums for categorical fields like occupancy levels and vehicle types to ensure type safety.
+from enum import Enum + +class OccupancyLevel(str, Enum): + LOW = "low" + MEDIUM = "medium" + HIGH = "high" + UNKNOWN = "unknown" + +class VehicleType(str, Enum): + IC = "IC" # InterCity + EC = "EC" # EuroCity + S = "S" # Suburban + @dataclass class LiveboardDeparture(DataClassORJSONMixin): """Details of a single departure in the liveboard response.""" # ... existing fields ... - vehicle_info: VehicleInfo = field(metadata=field_options(alias="vehicleinfo")) + vehicle_info: VehicleInfo = field( + metadata=field_options( + alias="vehicleinfo", + validate=lambda x: isinstance(x.type, VehicleType) + ) + )
107-240: Add default values for optional fields.Consider adding default values for fields that might be optional in the API response.
@dataclass class ConnectionStop(DataClassORJSONMixin): """Represents a single stop in a journey for connections.""" id: str station: str station_info: StationDetails = field(metadata=field_options(alias="stationinfo")) - scheduled_arrival_time: int = field(metadata=field_options(alias="scheduledArrivalTime")) + scheduled_arrival_time: int | None = field( + metadata=field_options(alias="scheduledArrivalTime"), + default=None + ) - arrival_canceled: bool = field(metadata=field_options(alias="arrivalCanceled")) + arrival_canceled: bool = field( + metadata=field_options(alias="arrivalCanceled"), + default=False + )
286-394: Group related boolean fields into feature classes.Consider grouping related boolean fields into separate feature classes for better organization.
+@dataclass +class ComfortFeatures: + """Represents comfort features available in a train unit.""" + has_toilets: bool + has_tables: bool + has_heating: bool + has_airco: bool + has_luggage_section: bool + +@dataclass +class PowerFeatures: + """Represents power outlet features in a train unit.""" + has_second_class_outlets: bool + has_first_class_outlets: bool + @dataclass class Unit(DataClassORJSONMixin): """Represents a single train unit, including its type and location.""" id: str material_type: MaterialType = field(metadata=field_options(alias="materialType")) - has_toilets: bool = field(metadata=field_options(alias="hasToilets")) - has_tables: bool = field(metadata=field_options(alias="hasTables")) - has_second_class_outlets: bool = field(metadata=field_options(alias="hasSecondClassOutlets")) - has_first_class_outlets: bool = field(metadata=field_options(alias="hasFirstClassOutlets")) - has_heating: bool = field(metadata=field_options(alias="hasHeating")) - has_airco: bool = field(metadata=field_options(alias="hasAirco")) - has_luggage_section: bool = field(metadata=field_options(alias="hasLuggageSection")) + comfort_features: ComfortFeatures + power_features: PowerFeatures
396-415: Use enum for disturbance types.Consider using an enum for the disturbance type field to ensure valid values.
+from enum import Enum + +class DisturbanceType(str, Enum): + DISTURBANCE = "disturbance" + PLANNED = "planned" + WORKS = "works" + ACCIDENT = "accident" + @dataclass class Disturbance(DataClassORJSONMixin): """Represents a railway system disturbance, including description and metadata.""" id: str title: str description: str link: str - type: str + type: DisturbanceType timestamp: int attachment: str | None = None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
examples/composition_from_docs_not_real.json(1 hunks)examples/connections.json(1 hunks)examples/disturbances_from_docs_not_real.json(1 hunks)examples/liveboard.json(1 hunks)examples/vehicle.json(1 hunks)pyrail/irail.py(12 hunks)pyrail/models.py(2 hunks)tests/test_irail.py(4 hunks)
🔇 Additional comments (11)
tests/test_irail.py (1)
82-90: LGTM! Clean test assertions.The test assertions are well-structured and validate the expected response format correctly.
pyrail/irail.py (1)
351-351: LGTM! Clean type annotation update.The return type annotation and response parsing have been correctly updated to use
StationDetailsinstead ofStation.Also applies to: 370-371
examples/disturbances_from_docs_not_real.json (1)
1-48: LGTM! Well-structured example JSON.The example JSON provides a comprehensive representation of both real-time and planned disturbances, with consistent structure and all necessary fields.
A few observations:
- Good inclusion of both disturbance types (real-time and planned)
- Consistent field structure across all entries
- Proper handling of multilingual content
- Clear distinction between regular disturbances and planned works
examples/composition_from_docs_not_real.json (1)
1-131: LGTM! Comprehensive train composition example.The example JSON provides detailed information about train composition with well-structured data for segments and units.
Notable aspects:
- Detailed unit specifications including amenities
- Clear hierarchy (composition > segments > units)
- Consistent boolean representation using "0"/"1"
- Comprehensive coverage of train features and capabilities
examples/liveboard.json (2)
1-12: LGTM! Well-structured metadata section.The metadata section provides comprehensive station information with proper versioning and timestamp.
13-239: Verify platform information availability.The departures section shows inconsistent platform information:
- Some platforms are marked with "?" which might indicate pending assignments
- The last departure (Antwerp-Central) has
"normal": "0"indicating a non-standard platform assignmentRun this script to check platform information patterns across the codebase:
pyrail/models.py (1)
Line range hint
1-18: LGTM! Well-structured base class implementation.The
ApiResponsebase class provides a solid foundation with proper documentation and JSON serialization support.examples/vehicle.json (2)
14-557: Ensure platform information consistency.The stops section shows inconsistencies:
- Most stops have "?" as platform information
- Some stops have specific platform numbers
- The final stop has
"normal": "0"indicating a non-standard platformRun this script to analyze platform information patterns:
1-13: Verify the default location coordinates.The vehicle location coordinates are set to "0", which might indicate:
- A placeholder for unknown location
- A vehicle that's not currently in service
- A potential data issue
Run this script to check location coordinate patterns:
✅ Verification successful
Zero coordinates are a valid default value
The "0,0" coordinates are used consistently throughout the codebase to indicate vehicles that are not currently being tracked or where position data is not available. This is a standard pattern in the API's data structure, appearing in multiple example files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for patterns in vehicle location coordinates # Search for zero coordinates pattern rg -A 2 '"locationX":\s*"0".*"locationY":\s*"0"' || echo "No zero coordinates found"Length of output: 111
Script:
#!/bin/bash # Find all vehicle-related JSON files echo "=== Vehicle JSON files ===" fd -e json vehicle # Check for any locationX/Y patterns echo -e "\n=== Location coordinate patterns ===" rg '"locationX":\s*"[^"]+"|"locationY":\s*"[^"]+"' -A 1Length of output: 86818
examples/connections.json (2)
1-4: LGTM! Consistent metadata structure.The metadata section follows the same structure as other response files, maintaining consistency across the API.
5-1387: Standardize platform information across connections.The connections section shows several inconsistencies:
- Platform information varies between specific numbers and "?"
- Some intermediate stops lack platform details
- Platform normality flags vary between stops
Run this script to analyze data consistency:
✅ Verification successful
Platform information format is consistent with API design
The use of "?" for intermediate stop platforms appears to be an intentional design choice:
- All platform fields are properly populated (no empty values)
- Unknown platforms are consistently represented with "?"
- Platform normality flags are present across all transportation data files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Analyze consistency patterns in connection data # Check platform information patterns echo "Platform patterns in connections:" rg -c '"platform":\s*"\?"' || echo "No questionable platforms found" rg -c '"normal":\s*"[01]"' || echo "No platform normality flags found" # Check for missing platform information echo -e "\nMissing platform information:" rg -c '"platform":\s*""' || echo "No empty platform fields found"Length of output: 416
|
@coderabbitai You don't need to review the data dumps in |
|
@tjorim You can exclude the example data dumps by adding a path filter in your CodeRabbit configuration. You can do this in two ways:
reviews:
path_filters:
- "!examples/**"
This will ensure that files in the 🔗 Analysis chain@tjorim I understand that you want to exclude the example data dumps from review. You can configure this in CodeRabbit by adding a path filter to exclude the 🏁 Scripts executedThe following scripts were executed for the analysis:
✏️ Learnings added
✅ Actions performedReview triggered.
|
|
@coderabbitai generate more tests |
|
Tip For best results, initiate chat on the files or code changes. @tjorim Here are additional tests for the pyrail project, focusing on the new models and their functionality: """Additional unit tests for the pyrail models and API endpoints."""
import pytest
from datetime import datetime
from pyrail.models import (
LiveboardApiResponse,
VehicleApiResponse,
CompositionApiResponse,
DisturbancesApiResponse,
StationDetails,
VehicleInfo,
PlatformInfo,
Occupancy,
)
# Sample data for testing
SAMPLE_STATION = {
"@id": "http://irail.be/stations/NMBS/008892007",
"id": "BE.NMBS.008892007",
"name": "Brussels-Central",
"locationX": 4.356802,
"locationY": 50.845658,
"standardname": "BRUXELLES-CENTRAL/BRUSSEL-CENTRAAL"
}
SAMPLE_VEHICLE = {
"name": "IC 538",
"shortname": "538",
"number": "538",
"type": "IC",
"locationX": 4.356802,
"locationY": 50.845658,
"@id": "http://irail.be/vehicle/IC538"
}
@pytest.mark.asyncio
async def test_liveboard_response_model():
"""Test LiveboardApiResponse model serialization and deserialization."""
sample_data = {
"version": "1.3",
"timestamp": int(datetime.now().timestamp()),
"station": "Brussels-Central",
"stationinfo": SAMPLE_STATION,
"departures": {
"number": 1,
"departure": [{
"id": "1",
"station": "Antwerp-Central",
"stationinfo": SAMPLE_STATION,
"time": int(datetime.now().timestamp()),
"delay": 0,
"canceled": False,
"left": False,
"isExtra": False,
"vehicle": "IC538",
"vehicleinfo": SAMPLE_VEHICLE,
"platform": "3",
"platforminfo": {"name": "3", "normal": True},
"occupancy": {"@id": "low", "name": "LOW"},
"departureConnection": "http://irail.be/connections/1"
}]
}
}
response = LiveboardApiResponse.from_dict(sample_data)
assert response.version == "1.3"
assert isinstance(response.station_info, StationDetails)
assert len(response.departures.departure) == 1
assert response.departures.departure[0].platform == "3"
# Test serialization
serialized = response.to_dict()
assert serialized["stationinfo"]["@id"] == SAMPLE_STATION["@id"]
assert serialized["departures"]["departure"][0]["isExtra"] is False
@pytest.mark.asyncio
async def test_vehicle_api_response():
"""Test VehicleApiResponse model with sample data."""
async with iRail() as api:
vehicle = await api.get_vehicle("IC538")
assert vehicle is not None
assert isinstance(vehicle, VehicleApiResponse)
assert isinstance(vehicle.vehicle_info, VehicleInfo)
assert vehicle.stops.number >= 0
# Test that all stops have valid platform info
for stop in vehicle.stops.stop:
assert isinstance(stop.platform_info, PlatformInfo)
assert isinstance(stop.occupancy, Occupancy)
@pytest.mark.asyncio
async def test_composition_api_response():
"""Test train composition endpoint and response model."""
async with iRail() as api:
composition = await api.get_composition("IC538")
assert composition is not None
assert isinstance(composition, CompositionApiResponse)
# Test segments structure
segments = composition.composition.segments
assert segments.number >= 0
if segments.number > 0:
segment = segments.segment[0]
assert isinstance(segment.origin, StationDetails)
assert isinstance(segment.destination, StationDetails)
# Test units in composition
units = segment.composition.units
assert units.number >= 0
if units.number > 0:
unit = units.unit[0]
assert isinstance(unit.has_toilets, bool)
assert isinstance(unit.seats_first_class, int)
assert isinstance(unit.length_in_meter, int)
@pytest.mark.asyncio
async def test_disturbances_api_response():
"""Test disturbances endpoint and response model."""
async with iRail() as api:
disturbances = await api.get_disturbances()
assert disturbances is not None
assert isinstance(disturbances, DisturbancesApiResponse)
# Test disturbance attributes
if len(disturbances.disturbances) > 0:
disturbance = disturbances.disturbances[0]
assert isinstance(disturbance.id, str)
assert isinstance(disturbance.title, str)
assert isinstance(disturbance.timestamp, int)
assert disturbance.type in ["disturbance", "planned"]
@pytest.mark.asyncio
async def test_error_handling():
"""Test error handling for various API endpoints."""
async with iRail() as api:
# Test with invalid vehicle ID
vehicle = await api.get_vehicle("INVALID_ID")
assert vehicle is None
# Test with invalid station for liveboard
liveboard = await api.get_liveboard("INVALID_STATION")
assert liveboard is None
# Test with invalid train ID for composition
composition = await api.get_composition("INVALID_TRAIN")
assert composition is None
@pytest.mark.asyncio
async def test_field_aliases():
"""Test that field aliases are correctly handled in serialization."""
station = StationDetails.from_dict({
"@id": "test_id",
"id": "BE.TEST.1",
"name": "Test Station",
"locationX": 4.0,
"locationY": 51.0,
"standardname": "TEST"
})
assert station.at_id == "test_id"
assert station.longitude == 4.0
assert station.latitude == 51.0
assert station.standard_name == "TEST"
# Test serialization maintains the original field names
serialized = station.to_dict()
assert "@id" in serialized
assert "locationX" in serialized
assert "locationY" in serialized
assert "standardname" in serializedThese tests cover:
Let me know if you'd like me to add more specific test cases or cover additional scenarios! 🔗 Analysis chain🏁 Scripts executedThe following scripts were executed for the analysis: Script: Length of output: 24282 |
|
@coderabbitai open a new PR for that |
|
Tip For best results, initiate chat on the files or code changes. @tjorim I've created a new pull request with the additional tests at #25. The PR includes comprehensive tests for all the new models and API endpoints introduced in #23. Feel free to review and provide feedback! 🔗 Analysis chain🏁 Scripts executedThe following scripts were executed for the analysis: Script: Length of output: 1484 Script: Length of output: 1571 |
|
@coderabbitai open a new PR to update pyrail/irail.py so it uses the new model data classes |
coderabbitai bot
pushed a commit
that referenced
this pull request
Jan 7, 2025
- Import all new response models - Update method signatures with proper return types - Convert raw dictionary responses to typed model instances - Update docstrings to reflect the new return types Related to #23
coderabbitai bot
pushed a commit
that referenced
this pull request
Jan 7, 2025
- Import all new response models - Update method signatures with proper return types - Convert raw dictionary responses to typed model instances - Update docstrings to reflect the new return types Related to #23
|
Tip For best results, initiate chat on the files or code changes. @tjorim I've created a new pull request at #26 that updates the iRail class to use the new model data classes. The PR includes all the necessary changes to properly integrate the new data models introduced in #23. Feel free to review the changes! 🔗 Analysis chain@tjorim I'll create a new PR to update the iRail class to use the new model data classes. The changes will include:
Let me create that PR for you now. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: Length of output: 39191 Script: Length of output: 2521 Script: Length of output: 1721 |
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.
Closes #19
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Improvements