Skip to content

Conversation

@Marenz
Copy link
Contributor

@Marenz Marenz commented Sep 9, 2024

No description provided.

@Marenz Marenz requested review from a team as code owners September 9, 2024 15:50
@github-actions github-actions bot added part:tests Affects the unit, integration and performance (benchmarks) tests part:cli Affects the command-line interface part:test-utils Affects the test utilities part:dispatcher labels Sep 9, 2024
Signed-off-by: Mathias L. Baumann <[email protected]>
Copy link

@daniel-zullo-frequenz daniel-zullo-frequenz left a comment

Choose a reason for hiding this comment

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

I have a few questions to check for. LGTM otherwise

@Marenz
Copy link
Contributor Author

Marenz commented Sep 9, 2024

updated

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 besides the small optional comments. The only thing that I would probably add is a test for receiving a dispatch with no duration, not sure if it is tested indirectly in the CLI test.

Comment on lines +28 to +29
# pylint: enable=no-name-in-module

Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't look right here, but you could also just disable it globally in the whole project as mypy will check for this anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is mostly at this place due to black

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, it is weird, it has to be just before the next line.

duration: timedelta | None
"""The duration of the dispatch, represented as a timedelta.
If None, the dispatch is considered to be "infinite" or "no-duration".
Copy link
Contributor

Choose a reason for hiding this comment

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

For me "no-duration" is not super clear, I think I would say "instantaneous" (or maybe both), or use an example as you did in the docs above.

Create and other places already use round(), so this
makes it more consistent.

Signed-off-by: Mathias L. Baumann <[email protected]>
Signed-off-by: Mathias L. Baumann <[email protected]>
@Marenz Marenz added this pull request to the merge queue Sep 12, 2024
Merged via the queue into frequenz-floss:v0.x.x with commit ff7e3f1 Sep 12, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants