Skip to content

Conversation

@Marenz
Copy link
Contributor

@Marenz Marenz commented Sep 29, 2025

This PR fixes a bug in the FakeService filter implementation where field existence was not properly checked before accessing optional protobuf fields. The fix replaces the walrus operator with explicit HasField() checks to correctly determine if optional fields are set.

@Marenz Marenz requested a review from a team as a code owner September 29, 2025 17:05
Copilot AI review requested due to automatic review settings September 29, 2025 17:05
@Marenz Marenz requested a review from a team as a code owner September 29, 2025 17:05
@Marenz Marenz requested a review from llucax September 29, 2025 17:05
@github-actions github-actions bot added part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests part:test-utils Affects the test utilities part:dispatcher labels Sep 29, 2025
Copy link

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 fixes a bug in the FakeService filter implementation where field existence was not properly checked before accessing optional protobuf fields. The fix replaces the walrus operator with explicit HasField() checks to correctly determine if optional fields are set.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
src/frequenz/client/dispatch/test/_service.py Updates filter logic to use HasField() instead of walrus operator for checking protobuf field existence
tests/test_service.py Adds comprehensive test coverage for start_time and end_time filtering functionality
RELEASE_NOTES.md Documents the bug fix in release notes

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +163 to 165
if dispatch.duration and (
dispatch.start_time + dispatch.duration < _to_dt(end_from)
):
Copy link

Copilot AI Sep 29, 2025

Choose a reason for hiding this comment

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

The condition check order has changed from the original logic. The original code checked dispatch.duration before the time comparison, but the parentheses now group the time comparison separately. Consider moving dispatch.duration check outside the parentheses to maintain the original short-circuit behavior: if dispatch.duration and dispatch.start_time + dispatch.duration < _to_dt(end_from):

Suggested change
if dispatch.duration and (
dispatch.start_time + dispatch.duration < _to_dt(end_from)
):
if dispatch.duration and dispatch.start_time + dispatch.duration < _to_dt(end_from):

Copilot uses AI. Check for mistakes.
@Marenz Marenz added this pull request to the merge queue Sep 30, 2025
Merged via the queue into frequenz-floss:v0.x.x with commit 8a1e92f Sep 30, 2025
5 checks passed
@Marenz Marenz deleted the service-fixes branch September 30, 2025 07:26
github-merge-queue bot pushed a commit to frequenz-floss/frequenz-dispatch-python that referenced this pull request Sep 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

2 participants