Skip to content

Add two new filters #167

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.

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

Open
wants to merge 2 commits into
base: v0.x.x
Choose a base branch
from
Open

Add two new filters #167

wants to merge 2 commits into from

Conversation

Marenz
Copy link
Contributor

@Marenz Marenz commented May 16, 2025

  • Reset release notes
  • Add two new filters to the dispatch list command

Signed-off-by: Mathias L. Baumann <[email protected]>
@Copilot Copilot AI review requested due to automatic review settings May 16, 2025 15:06
@Marenz Marenz requested review from a team as code owners May 16, 2025 15:06
@github-actions github-actions bot added part:docs Affects the documentation part:cli Affects the command-line interface part:dispatcher labels May 16, 2025
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR resets the release notes and enhances the dispatch list CLI command by adding two new filters.

  • Added --running and --type options to filter dispatches by status and type.
  • Updated filtering logic to count and report filtered-out dispatches.
  • Reset and stubbed out sections in RELEASE_NOTES.md.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
src/frequenz/client/dispatch/main.py Introduced --running and --type options, applied filtering in the loop, and updated summary output.
RELEASE_NOTES.md Cleared existing notes, added placeholders for Summary and Upgrading, and documented new filters.
Comments suppressed due to low confidence (1)

src/frequenz/client/dispatch/main.py:241

  • No tests cover the new --running and --type filters. Add unit or integration tests to verify that dispatches are correctly included or excluded based on these flags.
async for page in ctx.obj["client"].list(**filters):

@@ -218,28 +218,42 @@ async def cli(ctx: click.Context, url: str, key: str, raw: bool) -> None:
@click.option("--active", type=bool)
@click.option("--dry-run", type=bool)
@click.option("--page-size", type=int)
@click.option("--running", type=bool)
@click.option("--type", "-T", type=str)
Copy link
Preview

Copilot AI May 16, 2025

Choose a reason for hiding this comment

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

Using the option name --type shadows Python’s built-in type and may confuse users; consider renaming the flag (e.g. --dispatch-type) and setting dest='dispatch_type'.

Suggested change
@click.option("--type", "-T", type=str)
@click.option("--dispatch-type", "-T", type=str, dest="dispatch_type")

Copilot uses AI. Check for mistakes.

Comment on lines +221 to +222
@click.option("--running", type=bool)
@click.option("--type", "-T", type=str)
Copy link
Preview

Copilot AI May 16, 2025

Choose a reason for hiding this comment

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

Add a help= string for the new options (--running and --type) so users know exactly how to use them in the CLI.

Suggested change
@click.option("--running", type=bool)
@click.option("--type", "-T", type=str)
@click.option(
"--running",
type=bool,
help="Filter dispatches based on whether they are currently running.",
)
@click.option(
"--type",
"-T",
type=str,
help="Filter dispatches by their type (e.g., 'load', 'generation').",
)

Copilot uses AI. Check for mistakes.

Comment on lines +5 to +9
<!-- Here goes a general summary of what this release is about -->

## Upgrading

* You now must always provide the URL to the dispatch client.
<!-- Here goes notes on how to upgrade from previous versions, including deprecations and what they should be replaced with -->
Copy link
Preview

Copilot AI May 16, 2025

Choose a reason for hiding this comment

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

Placeholder comments remain in the Summary and Upgrading sections; fill these with actual release highlights and migration notes before publishing.

Copilot uses AI. Check for mistakes.

llucax
llucax previously approved these changes May 16, 2025
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, not sure if you have any tests for the CLI to add some tests for the additions...

@github-actions github-actions bot added the part:tests Affects the unit, integration and performance (benchmarks) tests label May 19, 2025
@Marenz Marenz requested a review from llucax May 19, 2025 10:38
@Marenz Marenz enabled auto-merge May 19, 2025 10:38
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:tests Affects the unit, integration and performance (benchmarks) tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants