chore: Update Disruption model fields to be optional #63
Conversation
Make the 'summary', 'additional_info', 'created', and 'last_update' fields in the Disruption model optional by setting their default value to None. This allows for more flexibility when creating instances of the Disruption class and handling data that may not always be available.
Reviewer's Guide by SourceryThis pull request updates the Disruption model to make the 'summary', 'additional_info', 'created', and 'last_update' fields optional by setting their default value to None. This change allows for more flexibility when creating instances of the Disruption class and handling data that may not always be available. File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @mnbf9rca - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| summary: Optional[str] = Field(None, alias='summary') | ||
| additional_info: Optional[str] = Field(None, alias='additionalInfo') | ||
| created: Optional[datetime] = Field(None, alias='created') | ||
| last_update: Optional[datetime] = Field(None, alias='lastUpdate') |
There was a problem hiding this comment.
issue: Handling of last_update being optional.
The last_update field is crucial for tracking changes. Making it optional could lead to issues in tracking the state of disruptions. Ensure that the logic accounts for None values appropriately.
| @@ -157,35 +157,35 @@ | |||
| "endpoint": "lineDisruptionsByLineId", | |||
| "endpoint_args": ["victoria,northern"], | |||
| "endpoint_params": {}, | |||
| "model": "Line", | |||
| "model": "Disruption", | |||
There was a problem hiding this comment.
suggestion (testing): Update test cases to reflect optional fields
Since the 'summary', 'additional_info', 'created', and 'last_update' fields in the Disruption model are now optional, it would be beneficial to add test cases that create Disruption instances without these fields to ensure the model handles these scenarios correctly.
| "model": "Disruption", | |
| "model": "Disruption", | |
| "summary": None, | |
| "additional_info": None, | |
| "created": None, | |
| "last_update": None, |
Make the 'summary', 'additional_info', 'created', and 'last_update' fields in the Disruption model optional by setting their default value to None. This allows for more flexibility when creating instances of the Disruption class and handling data that may not always be available.
Summary by Sourcery
This pull request updates the 'Disruption' model to make several fields optional, enhancing flexibility when creating instances. Additionally, test configurations were updated to align with these changes.