Skip to content

Conversation

@cwasicki
Copy link
Collaborator

The method is missing wrapping of the index and therefore lacks the essential functionality that the ring buffer should provide. Instead, the update function should be used. So far this method is only used in a very specific unit test.

@cwasicki cwasicki requested a review from a team as a code owner September 14, 2023 10:09
@cwasicki cwasicki requested a review from Marenz September 14, 2023 10:09
@cwasicki cwasicki self-assigned this Sep 14, 2023
@github-actions github-actions bot added part:tests Affects the unit, integration and performance (benchmarks) tests part:data-pipeline Affects the data pipeline labels Sep 14, 2023
@cwasicki cwasicki added this to the v1.0.0-rc2 milestone Sep 14, 2023
@github-actions github-actions bot added the part:docs Affects the documentation label Sep 14, 2023
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.

Seems reasonable to me, my only question is if it doesn't make sense to make __setitem__ use update underneath, to keep the buffer's standard interface, it might come handy at some point if interacting with generic functions, for example from the stdlib.

@cwasicki cwasicki force-pushed the setitem branch 2 times, most recently from 014ce7d to e70c93b Compare September 14, 2023 11:34
@cwasicki
Copy link
Collaborator Author

Updated.

my only question is if it doesn't make sense to make setitem use update underneath

In fact both methods are not doing the same. update takes a sample and determines the position in the buffer from the sample's timestamp. It also takes care of gap treatment. With __setitem you specify the position in the underlying buffer directly and bypass that logic, so you could add a value at any position without updating the buffer's internal state to function correctly.

We could still add the setter magic back in future if required, but at the moment I am not sure how exactly that should look like.

llucax
llucax previously approved these changes Sep 18, 2023
The method is missing wrapping of the index and therefore lacks the
essential functionality that the ring buffer should provide. Instead,
the `update` function should be used. So far this method is only used
in a very specific unit test.

Signed-off-by: cwasicki <[email protected]>
@cwasicki
Copy link
Collaborator Author

Resolved conflicts in release notes.

@cwasicki cwasicki added this pull request to the merge queue Sep 19, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 19, 2023
@llucax
Copy link
Contributor

llucax commented Sep 20, 2023

This was removed from the merge queue due to #662. Re-queuing.

@llucax llucax added this pull request to the merge queue Sep 20, 2023
Merged via the queue into frequenz-floss:v0.x.x with commit 036863a Sep 20, 2023
@cwasicki cwasicki deleted the setitem branch September 20, 2023 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

part:data-pipeline Affects the data pipeline part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests

Projects

Development

Successfully merging this pull request may close these issues.

2 participants