Update delivery time checks for gridpool streams and support delivery time filter in CLI#231
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request refactors the CLI and client code for electricity trading to improve the handling of delivery and execution time filters. The changes standardize parameter naming, add validation for delivery time intervals, and enhance output formatting.
Changes:
- Renamed CLI arguments from ambiguous
--start/--endto clearer--execution-from/--execution-toand--delivery-from/--delivery-to - Added
is_validproperty toDeliveryTimeFilterto validate that start time is before end time - Updated output formatting to include
gridpool_idin CSV headers for trades and orders
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/frequenz/client/electricity_trading/cli/__main__.py |
Updated CLI command definitions with renamed time filter arguments and parameter passing |
src/frequenz/client/electricity_trading/cli/etrading.py |
Refactored functions to use new parameter names, simplified filter creation logic, updated print functions to include gridpool_id, removed timezone import |
src/frequenz/client/electricity_trading/_types.py |
Added is_valid property to DeliveryTimeFilter class to validate time intervals |
src/frequenz/client/electricity_trading/_client.py |
Replaced validate_params calls with inline is_valid checks in streaming methods, updated docstrings |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """ | ||
| self.validate_params(delivery_time_filter=delivery_time_filter) | ||
| if delivery_time_filter and not delivery_time_filter.is_valid: | ||
| raise ValueError("Invalid delivery_time_filter provided.") |
There was a problem hiding this comment.
The error message "Invalid delivery_time_filter provided." is not very informative about what makes the filter invalid. Consider providing a more descriptive error message that explains the validation failure, such as "Invalid delivery_time_filter: end time must be after start time" to help users understand and fix the issue.
| raise ValueError("Invalid delivery_time_filter provided.") | |
| raise ValueError( | |
| "Invalid delivery_time_filter provided: ensure the start time is " | |
| "before the end time and that all required fields are set." | |
| ) |
| """ | ||
| self.validate_params(delivery_time_filter=delivery_time_filter) | ||
| if delivery_time_filter and not delivery_time_filter.is_valid: | ||
| raise ValueError("Invalid delivery_time_filter provided.") |
There was a problem hiding this comment.
The error message "Invalid delivery_time_filter provided." is not very informative about what makes the filter invalid. Consider providing a more descriptive error message that explains the validation failure, such as "Invalid delivery_time_filter: end time must be after start time" to help users understand and fix the issue.
| raise ValueError("Invalid delivery_time_filter provided.") | |
| raise ValueError( | |
| "Invalid delivery_time_filter: please ensure that the start time " | |
| "is before the end time and that all required fields are set." | |
| ) |
cwasicki
force-pushed
the
cli
branch
3 times, most recently
from
aa5f430 to
aa6b47f
Compare
matthias-wende-frequenz
left a comment
matthias-wende-frequenz
left a comment
There was a problem hiding this comment.
two small documentation suggestions
| gid: Gridpool ID. | ||
| delivery_start: Start of the delivery period or None. | ||
| delivery_from: Start of the delivery period or None. | ||
| delivery_to: End of the delivery period or None. |
There was a problem hiding this comment.
The documentation doesn't reflect the definition of the DeliveryTimeFilter.
| @click.option("--auth_key", required=True, type=str) | ||
| @click.option("--gid", required=True, type=int) | ||
| @click.option("--start", default=None, type=iso) | ||
| @click.option("--delivery-from", default=None, type=iso) |
There was a problem hiding this comment.
I suggest to add a help argument.
If both are set, start timestamps have to be before end timestamps. Signed-off-by: cwasicki <126617870+cwasicki@users.noreply.github.com>
Uses the new `is_valid` check that now comes with the delivery time filter. This also drops the check that delivery start time of the filter has to be in the future, which is not desired for the gridpool data streaming endpoints. Signed-off-by: cwasicki <126617870+cwasicki@users.noreply.github.com>
Renaming time range CLI arguments to use from/to instead of start/end to clearly distinguish from delivery start. Signed-off-by: cwasicki <126617870+cwasicki@users.noreply.github.com>
Adds the gridpool ID as a column to the CSV output which is useful when outputs from multiple gridpools are merged. Signed-off-by: cwasicki <126617870+cwasicki@users.noreply.github.com>
Replaces the delivery period filter to support full delivery period start intervals. Using to/from arguments to avoid ambiguity with delivery start. Signed-off-by: cwasicki <126617870+cwasicki@users.noreply.github.com>
Replaces the delivery period filter to support full delivery period start intervals. Using to/from arguments to avoid ambiguity with delivery start. Signed-off-by: cwasicki <126617870+cwasicki@users.noreply.github.com>
Signed-off-by: cwasicki <126617870+cwasicki@users.noreply.github.com>
Signed-off-by: cwasicki <126617870+cwasicki@users.noreply.github.com>
|
@matthias-wende-frequenz Updated |
|
@matthias-wende-frequenz Good? |
This pull request refactors and improves the handling of delivery and execution time filters in the electricity trading CLI and client code. It standardizes the naming of time-related parameters, enhances validation for delivery time filters, and updates the CLI and output formatting to be more consistent and informative.
Parameter naming and CLI improvements:
--start/--endwith clearer--execution-from,--execution-to,--delivery-from, and--delivery-toacross relevant commands incli/__main__.pyandcli/etrading.py. This clarifies the distinction between execution and delivery periods and improves usability.Validation and robustness:
is_validproperty to theDeliveryTimeFilterclass to check that the delivery time interval is logical (start < end), and updated the streaming methods in_client.pyto raise aValueErrorif an invalid filter is provided. This prevents invalid queries and improves error messaging.Output formatting and clarity:
gridpool_idto CSV headers and output, and updated the corresponding print functions to include this information. This makes CSV exports more informative and easier to analyze.Code simplification and cleanup:
Other minor changes:
timezone) to clean up the code.