Add the public order book extension to the Client#120
Conversation
ghost
self-requested a review
as a 
github-actions
Bot
added
part:docs
There was a problem hiding this comment.
Pull Request Overview
This PR integrates the Public Order Book extension into the Client. It adds new types PublicOrder and PublicOrderFilter, implements a new endpoint to stream public orders, and updates the dependency for the electricity trading API.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/frequenz/client/electricity_trading/_types.py | Added PublicOrder and PublicOrderFilter types with relevant conversion methods. |
| src/frequenz/client/electricity_trading/_client.py | Introduced the receive_public_orders endpoint and maintained the streaming logic for public orders. |
| pyproject.toml | Updated the dependency for frequenz-api-electricity-trading to the new version constraints. |
| RELEASE_NOTES.md | Updated release notes to document dependency changes and the addition of the Public Order Book extension. |
Comments suppressed due to low confidence (1)
src/frequenz/client/electricity_trading/_client.py:977
- The endpoint function name 'receive_public_orders' is inconsistent with the PR description using the singular 'receive_public_order()'. Consider renaming for consistency.
def receive_public_orders(
matthias-wende-frequenz
left a comment
matthias-wende-frequenz
left a comment
There was a problem hiding this comment.
For now I've added two small comments. Can you also please add tests?
|
|
||
|
|
||
| @dataclass() | ||
| class PublicOrderFilter: |
There was a problem hiding this comment.
I suggest to call it PublicOrderBookFilter, because it's used to filter entries in the order book and not filter single orders.
There was a problem hiding this comment.
Given that in the Client it's called PublicOrder I wouldn't. Or we write add *Book everywhere but imo for consistency in the API it's best to not include it in the names.
There was a problem hiding this comment.
In the client a single entry is called a public order. This class is used as a parameter for an rpc call to query a list of public orders.
| raise | ||
| return self._public_trades_streams[public_trade_filter] | ||
|
|
||
| def receive_public_orders( |
There was a problem hiding this comment.
I suggest to call this receive_public_order_book and not differ from API terminology.
There was a problem hiding this comment.
This receives a stream of public orders, which together form a order book. We should be consistent across client and API. (Also this is a term this is officially in use so we should stick to established terminology).
Ah yes good call, will do! |
* Add the PublicOrder and PublicOrderFilter types * Add the receive_public_order() endpoint * Update the frequenz-api-electricity-trading to >= 0.6.1, < 0.7.0 Signed-off-by: camille-bouvy-frequenz <camille.bouvy@frequenz.com>
Signed-off-by: camille-bouvy-frequenz <camille.bouvy@frequenz.com>
Signed-off-by: camille-bouvy-frequenz <camille.bouvy@frequenz.com>
github-actions
Bot
added
the
part:tests
|
Fixed the names (as discussed in previous comments and in our call) and added tests. |
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Signed-off-by: Matthias Wende <60187632+matthias-wende-frequenz@users.noreply.github.com>
This PR integrates the Public Order Book extension to the
Client. More specifically, the changes made are:PublicOrderandPublicOrderFiltertypesreceive_public_order()endpointfrequenz-api-electricity-tradingto >= 0.6.1, < 0.7.0