Support listing gridpool orders without price or quantity#182
Conversation
github-actions
Bot
added
part:docs
There was a problem hiding this comment.
Pull Request Overview
This PR removes validation that previously required gridpool orders to have valid price and quantity fields, allowing orders with missing or invalid price/quantity to be processed without raising exceptions. This change enables listing of order sets that contain some orders with missing data.
- Removed validation logic that checked for missing price or quantity in non-canceled orders
- Updated tests to reflect the new behavior where missing fields no longer raise exceptions
- Added release notes documenting the breaking change
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/frequenz/client/electricity_trading/_types.py | Removed validation logic and exception raising for orders with missing price/quantity |
| tests/test_types.py | Updated tests to remove exception assertions and added comments explaining the behavior change |
| RELEASE_NOTES.md | Added upgrade note about the behavior change requiring user validation |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| # Missing price | ||
| od_pb1 = electricity_trading_pb2.OrderDetail() | ||
| od_pb1.CopyFrom(ORDER_DETAIL_PB) | ||
| OrderDetail.from_pb(od_pb1) |
There was a problem hiding this comment.
The test calls OrderDetail.from_pb(od_pb1) before clearing the price field, but this call doesn't seem to serve any purpose in testing the missing price scenario. Consider removing line 558 or adding an assertion to clarify its intent.
| OrderDetail.from_pb(od_pb1) | |
| # Removed unnecessary call before clearing price |
There was a problem hiding this comment.
Not fully agree on that but if @matthias-wende-frequenz prefers I am not insisting and can remove it.
| # Missing quantity (same logic as above) | ||
| od_pb2 = electricity_trading_pb2.OrderDetail() | ||
| od_pb2.CopyFrom(ORDER_DETAIL_PB) | ||
| OrderDetail.from_pb(od_pb2) |
There was a problem hiding this comment.
Similar to the price test, this calls OrderDetail.from_pb(od_pb2) before clearing the quantity field without any apparent purpose. Consider removing line 567 or adding an assertion to clarify its intent.
| OrderDetail.from_pb(od_pb2) | |
| # Removed unnecessary call to OrderDetail.from_pb(od_pb2) |
matthias-wende-frequenz
left a comment
matthias-wende-frequenz
left a comment
There was a problem hiding this comment.
Looks good apart from the two copilot comments
| # Missing price | ||
| od_pb1 = electricity_trading_pb2.OrderDetail() | ||
| od_pb1.CopyFrom(ORDER_DETAIL_PB) | ||
| OrderDetail.from_pb(od_pb1) |
When listing, gridpool orders that do not contain a price or quantity were previously considered invalid and raised an exception. Since this prevented listing any set of orders where at least one order was considered invalid, this has been changed and users have to validate orders themselves. Signed-off-by: cwasicki <126617870+cwasicki@users.noreply.github.com>
Signed-off-by: cwasicki <126617870+cwasicki@users.noreply.github.com>
When listing, gridpool orders that do not contain a price or quantity were previously considered invalid and raised an exception. Since this prevented listing any set of orders where at least one order was considered invalid, this has been changed and users have to validate orders themselves.