Handle optional fields in dataclasses #32
Conversation
📝 WalkthroughWalkthroughThe pull request introduces modifications to the data models in the Changes
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 (
|
![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
🧹 Nitpick comments (3)
pyrail/models.py (3)
84-84: Consider adding a default value for consistency.While the change to make
occupancyoptional is good, consider adding an explicit default value usingfield(default=None)for consistency with other optional fields in the codebase (like in VehicleStop class).- occupancy: Optional[Occupancy] # Occupancy level + occupancy: Optional[Occupancy] = field(default=None) # Occupancy level
265-270: Simplify default value implementation.Using
default_factory=lambda: Noneis unnecessarily complex. SinceNoneis a singleton, you can simply usedefault=None.- occupancy: Optional[Occupancy] = field( - default_factory=lambda: None, metadata=field_options(alias="occupancy") - ) # Occupancy level + occupancy: Optional[Occupancy] = field( + default=None, metadata=field_options(alias="occupancy") + ) # Occupancy level - departure_connection: Optional[str] = field( - default_factory=lambda: None, metadata=field_options(alias="departureConnection") - ) # Departure connection link + departure_connection: Optional[str] = field( + default=None, metadata=field_options(alias="departureConnection") + ) # Departure connection link
Line range hint
84-351: Consider standardizing optional field declarations across the codebase.There are three different patterns for declaring optional fields in this file:
field: Optional[Type](no default)field: Optional[Type] = field(default=None)field: Optional[Type] = field(default_factory=lambda: None)Consider standardizing on
field(default=None)pattern across all optional fields for better maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pyrail/models.py(4 hunks)
🔇 Additional comments (2)
pyrail/models.py (2)
4-4: LGTM!The
Optionalimport is correctly added to support the type hint changes.
346-351: Simplify default value implementation and verify consumers.
Using
default_factory=lambda: Noneis unnecessarily complex. SinceNoneis a singleton, you can simply usedefault=None.This change from
booltoOptional[bool]might affect code that consumes these fields. Ensure that consumers handle theNonecase appropriately.- has_tables: Optional[bool] = field( - default_factory=lambda: None, metadata=field_options(alias="hasTables") - ) # Whether the unit has tables + has_tables: Optional[bool] = field( + default=None, metadata=field_options(alias="hasTables") + ) # Whether the unit has tables - has_luggage_section: Optional[bool] = field( - default_factory=lambda: None, metadata=field_options(alias="hasLuggageSection") - ) # Whether the unit has a luggage section + has_luggage_section: Optional[bool] = field( + default=None, metadata=field_options(alias="hasLuggageSection") + ) # Whether the unit has a luggage sectionLet's verify the impact on consumers:
|
Thanks. Already addressed in 557a1c7. Since liveboard endpoint returns departures, it always has an occupancy. The last stop of a vehicle does not (iRail/iRail#548 (comment)). hasTables has been confirmed to be not in the data (iRail/iRail#549 (comment)), I will assume this also applies to hasLuggageSection. Also did not see materialNumber or materialSubTypeName in the actual response. |
|
I forgot we can also query liveboard arrivals which also contains different data: #33 |
Summary by CodeRabbit