Move some high-level functions to this repo #103
Conversation
Marenz
requested review from
ela-kotulska-frequenz and
hannah-stevenson-frequenz
github-actions
bot
added
part:docs
There was a problem hiding this comment.
About the RunningState/is_running(), I know this PR is not about refactoring but just moving code, so I would be OK to just with that as it is, but I would help myself bringing it up. Is just too weird, and I think it is even weirder in the context of this library.
|
|
||
| return pb_rule | ||
|
|
||
| def prepare(self, start_time: datetime) -> rrule.rrule: |
There was a problem hiding this comment.
This method names sounds super strange to me. If I understand correctly this method is converting the RecurrenceRule object to a rrule object. If this is correct, I would name this as_rrule() to be more familiar with what we use in quantities for example, or to_rrule(), to match the to_protobuf(). I'm also wondering if we really want to make rrule part of the public API. I don't remember how do we use the rrule, but maybe we can expose rrule functionality directly without exposing that we are using rrule internally?
Not a big deal though, I guess the client is still pretty low level so if it makes sense I'm fine with just returning the rrule.
There was a problem hiding this comment.
Well, this method is pretty much only used with in the public facing methods of Dispatch, we can also make it private.
I am fine with either of your name suggestions otherwise..
There was a problem hiding this comment.
OK, I see. I guess it is the same with from and to_protobuf, they should also probably be private. What I ended up doing in the microgrid API was to just put conversion functions for protobuf in a separate (private) file, not as methods. This way the interface used by the end user doesn't need to know anything about protobuf, not even import the protobuf stuff, and also you don't need to add # pylint: disable-... when calling private methods. The implementation needed to do the conversion can just import the private module and that's it. For example microgrid_info.py and _microgrid_info_proto.py.
So, not for this PR but an idea for the future. As of what to do now, I think the lowest hanging fruit would be to just rename to make it more clear and leave it public, as the other internal methods are also public currently, so if we want to fix it, all will need to be fixed.
There was a problem hiding this comment.
Once more I just noticed you moved forward with updating the code before I can reply, so probably my reply there doesn't make sense anymore 😆
| def running(self, type_: str) -> RunningState: | ||
| """Check if the dispatch is currently supposed to be running. | ||
| Args: | ||
| type_: The type of the dispatch that should be running. | ||
| Returns: | ||
| RUNNING if the dispatch is running, | ||
| STOPPED if it is stopped, | ||
| DIFFERENT_TYPE if it is for a different type. | ||
| """ | ||
| if self.type != type_: | ||
| return RunningState.DIFFERENT_TYPE | ||
|
|
||
| if not self.active: | ||
| return RunningState.STOPPED | ||
|
|
||
| now = datetime.now(tz=timezone.utc) | ||
|
|
||
| if now < self.start_time: | ||
| return RunningState.STOPPED | ||
|
|
||
| # A dispatch without duration is always running, once it started | ||
| if self.duration is None: | ||
| return RunningState.RUNNING | ||
|
|
||
| if until := self._until(now): | ||
| return RunningState.RUNNING if now < until else RunningState.STOPPED | ||
|
|
||
| return RunningState.STOPPED |
There was a problem hiding this comment.
Can't we just make this a bool and forget about checking the type here? While we are at it, make it a property and rename to is_active. Running seems to strong here, that this library is not really running anything.
There was a problem hiding this comment.
I am very much against renaming this to is_active, else it can extremely easily be confused with the dispatch attribute active..
We can change it to a property def started()?
There was a problem hiding this comment.
I am very much against renaming this to
is_active, else it can extremely easily be confused with the dispatch attributeactive..
😬
We can change it to a property
def started()?
I don't like it that much, but it would match the new proposed status name, so I guess we can go with that, and when status is implemented, we can just have a status property and that's it.
PS: Copilot suggested in progress, which I like better, but let's discuss status names in the status issue.
There was a problem hiding this comment.
No, the status property is very different from this and should be kept very separate, semantically as well.
There was a problem hiding this comment.
we could call it "should_be_running"
There was a problem hiding this comment.
or "should_target_be_running"?
There was a problem hiding this comment.
No, the status property is very different from this and should be kept very separate, semantically as well.
Why?
There was a problem hiding this comment.
Well, the status reflects something based on reality (optimally), where as the dispatches "is_running" or however we call it is purely based on what the dispatch wants to happen.
There was a problem hiding this comment.
But the dispatch will still be the source of truth I guess, and its data will be sent to the API at some point so it is based on reality. In any case, I think it makes no sense to push for this now and the status interface is still being discussed. For the sake of advancing with this PR, I'm OK with keeping is_running for now, which is basically what we had before.
What's more important now IMHO is to make this a property and kill DIFFERENT_TYPE, if this can be done in this PR (so the client doesn't have to ever have DIFFERENT_TYPE) then great, otherwise we can leave it as a future improvement.
Signed-off-by: Mathias L. Baumann <[email protected]>
This moves some functionality from the high-level interface to the client. Signed-off-by: Mathias L. Baumann <[email protected]>
Signed-off-by: Mathias L. Baumann <[email protected]>
"dispatch" is redundant in this context. Signed-off-by: Mathias L. Baumann <[email protected]>
Signed-off-by: Mathias L. Baumann <[email protected]>
Signed-off-by: Mathias L. Baumann <[email protected]>
Signed-off-by: Mathias L. Baumann <[email protected]>
There was a problem hiding this comment.
Seeing now that started also takes into account the active property, I'm starting to like your suggestion about should_be_running. ChatGPT suggested in the past also something like is_active_now to disambiguate with the plain active, and I think it might make sense too, as should_be_running also makes it sound like it should but it isn't. I like active as it doesn't sound so tied to execution like started or running.
All that said, as it seems none of the options are perfect, I'm more than happy to keep started, it is enough, so feel free to fix the release notes and self-merge afterwards.
RELEASE_NOTES.md
Outdated
| ## New Features | ||
|
|
||
| * Update BaseApiClient to get the http2 keepalive feature. | ||
| * Some Methods from the high-level API have been moved to this repo: The dispatch class now offers: `until`, `running`, `next_run` and `next_run_after`. |
There was a problem hiding this comment.
| * Some Methods from the high-level API have been moved to this repo: The dispatch class now offers: `until`, `running`, `next_run` and `next_run_after`. | |
| * Some Methods from the high-level API have been moved to this repo: The dispatch class now offers: `until`, `started`, `next_run` and `next_run_after`. |
Signed-off-by: Mathias L. Baumann <[email protected]>
2 participants
Having these methods available in this repo allows us to provide more convenient
and useful information in the dispatch-cli client.