Skip to content
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

Mock resampler: Improve variable names #879

Merged
merged 1 commit into from
Feb 14, 2024

Conversation

llucax
Copy link
Contributor

@llucax llucax commented Feb 14, 2024

Since the mock resampler uses a pair of channels, one to receive samples to be (fakely) resampled (input) and one to send the (fakely) resampmled samples (output), we should use clear names to identify both otherwise is really hard to follow which is which when reading the code.

Since the mock resampler uses a pair of channels, one to receive
samples to be (fakely) resampled (input) and one to send the (fakely)
resampmled samples (output), we should use clear names to identify both
otherwise is really hard to follow which is which when reading the code.

Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
@llucax llucax requested a review from a team as a code owner February 14, 2024 15:06
@llucax llucax requested a review from Marenz February 14, 2024 15:06
@github-actions github-actions bot added the part:tests Affects the unit, integration and performance (benchmarks) tests label Feb 14, 2024
@llucax llucax self-assigned this Feb 14, 2024
@llucax llucax added this to the v1.0.0-rc5 milestone Feb 14, 2024
@llucax llucax added the type:tech-debt Improves the project without visible changes for users label Feb 14, 2024
@llucax
Copy link
Contributor Author

llucax commented Feb 14, 2024

Enabled auto-merge.

@llucax llucax enabled auto-merge February 14, 2024 15:09
@llucax llucax added this pull request to the merge queue Feb 14, 2024
Merged via the queue into frequenz-floss:v1.x.x with commit 03e6a3f Feb 14, 2024
15 checks passed
@llucax llucax deleted the better-var-names branch February 14, 2024 15:53
Comment on lines +225 to +232
input_chan_recv_name = f"{request.component_id}:{request.metric_id}"
input_chan_recv = self._input_channels_receivers[input_chan_recv_name].pop()
assert input_chan_recv is not None
output_chan_sender: Sender[Sample[Quantity]] = (
self._channel_registry.get_or_create(
Sample[Quantity], name
).new_sender()
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, this is better than basic_receivers, but input_receivers and output_sender might have even nicer. input_channels_receivers make me think that they receive channels and not input values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, at first I named it without the chan, but for some reason it still seemed ambiguous, but now I don't remember why. Maybe @daniel-zullo-frequenz as he was helping me to understand the code while I did it.

Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC we wanted to mainly get rid of basic in the variable name because it was confusing/difficult-to-understand what a basic_receiver was. I do agree having only receivers and sender in the name would have suffice to understand they are channels given the context. However I don't think it causes any misunderstanding if we keep chan/channels explicitly in the variable names. I mean I haven't seen yet any scenarios where we send/receive channels

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
part:tests Affects the unit, integration and performance (benchmarks) tests type:tech-debt Improves the project without visible changes for users
Projects
Development

Successfully merging this pull request may close these issues.

4 participants