Skip to content

Update API and add support for start_immediately #105

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Nov 14, 2024
Merged

Update API and add support for start_immediately #105

merged 3 commits into from
Nov 14, 2024

Conversation

@Marenz
Copy link
Contributor

@Marenz Marenz commented Nov 7, 2024

  • Update API dependency
  • CLI: Change interval default to 1
  • Add support for start_immediately

Marenz added 2 commits November 7, 2024 16:44
@Marenz
Update API dependency
91a86f3 
* selector was renamed to target
* start_immediately was added in the proto rpc create

Signed-off-by: Mathias L. Baumann <[email protected]>

# Conflicts:
#	src/frequenz/client/dispatch/_client.py
#	src/frequenz/client/dispatch/_internal_types.py
#	src/frequenz/client/dispatch/types.py
#	tests/test_proto.py
@Marenz
CLI: Change interval default to 1
8e97d4f 
We basically never want 0 here.

Signed-off-by: Mathias L. Baumann <[email protected]>
@Marenz Marenz requested review from a team as code owners November 7, 2024 15:45
@github-actions github-actions bot added part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests part:tooling Affects the development tooling (CI, deployment, dependency management, etc.) part:cli Affects the command-line interface part:test-utils Affects the test utilities part:dispatcher labels Nov 7, 2024
llucax
llucax previously approved these changes Nov 8, 2024
View reviewed changes
Copy link
Contributor

@llucax llucax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just 2 small optional suggestions.

I would use a sentinel1 instead of Literal["NOW"], it seems less error prone and less typing, but this was not introduced by this PR, so for a separate PR I guess.

Footnotes

  1. I think we should add the reference implementation to core. ↩

src/frequenz/client/dispatch/_cli_types.py Outdated
Comment on lines 37 to 38
if isinstance(value, datetime) or value == "NOW":
return cast(datetime | Literal["NOW"], value)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you are also returning Literal["NOW"] below to do a more relaxed comparison to "NOW", wouldn't it be easier and avoid the cast to just do this here?

Suggested change
if isinstance(value, datetime) or value == "NOW":
return cast(datetime | Literal["NOW"], value)
if isinstance(value, datetime):
return value

src/frequenz/client/dispatch/_cli_types.py Outdated
return cast(datetime | Literal["NOW"], value)

try:
if value in ("NOW", "now"):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if value in ("NOW", "now"):
if value.upper() == "NOW":

@llucax
Copy link
Contributor

llucax commented Nov 8, 2024

  1. I think we should add the reference implementation to core. ↩

@Marenz
Add support for start_immediately
5db3b6c 
Signed-off-by: Mathias L. Baumann <[email protected]>
@Marenz Marenz merged commit ec026f9 into frequenz-floss:v0.x.x Nov 14, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@llucax llucax llucax left review comments

@stefan-brus-frequenz stefan-brus-frequenz Awaiting requested review from stefan-brus-frequenz stefan-brus-frequenz is a code owner automatically assigned from frequenz-floss/api-dispatch-team

@ela-kotulska-frequenz ela-kotulska-frequenz Awaiting requested review from ela-kotulska-frequenz ela-kotulska-frequenz is a code owner automatically assigned from frequenz-floss/python-sdk-team

Assignees

No one assigned

Labels

part:cli Affects the command-line interface part:dispatcher part:docs Affects the documentation part:test-utils Affects the test utilities part:tests Affects the unit, integration and performance (benchmarks) tests part:tooling Affects the development tooling (CI, deployment, dependency management, etc.)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Marenz @llucax