Skip to content

Conversation

@Marenz
Copy link
Contributor

@Marenz Marenz commented Jun 12, 2025

No description provided.

Copilot AI review requested due to automatic review settings June 12, 2025 18:02
@Marenz Marenz requested a review from a team as a code owner June 12, 2025 18:03
@github-actions github-actions bot added the part:dispatcher Affects the high-level dispatcher interface label Jun 12, 2025
@Marenz Marenz added the cmd:skip-release-notes It is not necessary to update release notes for this PR label Jun 12, 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 refactors the component-target extraction logic by moving it from a private class method into a standalone helper function and updates the actor start path to use the new helper.

  • Removed _get_target_components_from_dispatch method
  • Added module-level _convert_target_components function
  • Updated _start_actor to call _convert_target_components
Comments suppressed due to low confidence (2)

src/frequenz/dispatch/_actor_dispatcher.py:331

  • Add a docstring for _convert_target_components explaining its purpose, parameters, and return values to improve readability and maintainability.
def _convert_target_components(target: ClientTargetComponents) -> TargetComponents:

src/frequenz/dispatch/_actor_dispatcher.py:331

  • Introduce unit tests for _convert_target_components to verify correct behavior when target contains integer IDs versus ComponentCategory values.
def _convert_target_components(target: ClientTargetComponents) -> TargetComponents:

@Marenz Marenz enabled auto-merge June 12, 2025 18:11
@llucax
Copy link
Contributor

llucax commented Jun 13, 2025

Oh, I reviewed this in #155, maybe mark PRs that depend on other PRs as draft?

@llucax
Copy link
Contributor

llucax commented Jun 13, 2025

I guess in this PR you didn't update the client and you can't use the new match-friendly target classes?

@Marenz
Copy link
Contributor Author

Marenz commented Jun 16, 2025

Oh, I reviewed this in #155, maybe mark PRs that depend on other PRs as draft?

I wanted to mark it as draft, but couldn't find the button/function anymore in the UI...
But I did think the WIP commit and the blocked tag gave it away

@Marenz
Copy link
Contributor Author

Marenz commented Jun 16, 2025

I guess in this PR you didn't update the client and you can't use the new match-friendly target classes?

Yeah, doing the BaseId update is a whole different thing again

@llucax
Copy link
Contributor

llucax commented Jun 17, 2025

But I did think the WIP commit and the blocked tag gave it away

You overestimate me...

@Marenz Marenz closed this Jun 27, 2025
auto-merge was automatically disabled June 27, 2025 08:39

Pull request was closed

@Marenz Marenz deleted the more-modular branch June 27, 2025 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cmd:skip-release-notes It is not necessary to update release notes for this PR part:dispatcher Affects the high-level dispatcher interface

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants