Add is_valid check to OrderDetail #180
Conversation
github-actions
bot
added
the
part:tests
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for optionally listing invalid orders in the gridpool orders endpoint. Previously, the system would raise an error when encountering orders with missing price or quantity fields for non-canceled orders. Now, these orders are considered "invalid" but can still be returned if requested.
- Modified
OrderDetail.from_pb()to no longer raise exceptions for invalid orders - Added
is_validproperty toOrderDetailto check order validity - Added
valid_onlyparameter tolist_gridpool_orders()to control filtering behavior
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 exception raising from from_pb() and added is_valid property for validation |
| src/frequenz/client/electricity_trading/_client.py | Added valid_only parameter to filter orders based on validity |
| tests/test_types.py | Updated tests to use is_valid property instead of expecting exceptions |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| Raises: | ||
| ValueError: If the order price and quantity are not specified for a non-canceled order. | ||
| """ |
There was a problem hiding this comment.
The docstring should be updated to mention that invalid orders are now supported and describe when an order is considered invalid.
| tag: The tag to filter by. | ||
| page_size: The number of orders to return per page. | ||
| timeout: Timeout duration, defaults to None. | ||
| valid_only: If True, only returns orders that are valid. |
There was a problem hiding this comment.
The parameter documentation should explain what makes an order valid or invalid (e.g., 'If True, only returns orders with finite price and quantity, unless they are cancelled').
| valid_only: If True, only returns orders that are valid. | |
| valid_only: If True, only returns orders with finite price and quantity, unless they are cancelled. If False, returns all orders regardless of validity. |
|
|
||
| @property | ||
| def is_valid(self) -> bool: | ||
| """Check if the order detail is valid. |
There was a problem hiding this comment.
The docs should tell what is considered as a valid order.
| tag: str | None = None, | ||
| page_size: int | None = None, | ||
| timeout: timedelta | None = None, | ||
| valid_only: bool = True, |
There was a problem hiding this comment.
Having the check only in this function is not sufficient, since there are other methods using OrderDetail. I wonder if from_pb method should keep the validation check but only raise if this flag is true.
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 <[email protected]>
Signed-off-by: cwasicki <[email protected]>
Can be used by the user to check the validity of an order, which is currently defined as any order with a valid price and quantity, or in cancelled state. Signed-off-by: cwasicki <[email protected]>
cwasicki
changed the title
|
After updating the docs and rebasing this is good to go. @cwasicki can you please take care of it? |
2 participants
Draft moving previous validity checks into a method of
OrderDetail. May be interesting for downstream users.