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

Add recent_index, oldest_index, oldest, and is_filled to HistoryBuffer API #347

Merged
merged 6 commits into from
Dec 20, 2023

Conversation

YuhanLiin
Copy link
Contributor

I added some convenience methods to the HistoryBuffer API to help keep track of both ends of the buffer.

@pleepleus
Copy link
Contributor

I was just about to fork this repo because I needed these. I will be happy to review.

Copy link
Contributor

@pleepleus pleepleus left a comment

Choose a reason for hiding this comment

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

The code changes themselves look excellent. In fact I already pointed my project to this commit because I needed the "oldest()" api and it passes my application tests.

Before this can be merged though it should:

  1. Refresh with a merge from main
  2. Modify the changelog with a note on the additions

Copy link
Member

@newAM newAM left a comment

Choose a reason for hiding this comment

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

Sorry for the slow review! Life has been keeping me busy lately.

src/histbuf.rs Outdated Show resolved Hide resolved
@newAM
Copy link
Member

newAM commented Dec 19, 2023

Thanks for making the change!

I took a closer look before merging, and I have one last question (sorry!). What is the use-case of the _index methods being public? That seems like an implementation detail.

@YuhanLiin
Copy link
Contributor Author

The index methods can be used with as_slice (which also seems like an implementation detail) to do indexing operations relative to the head and tail indices.

Copy link
Member

@newAM newAM left a comment

Choose a reason for hiding this comment

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

Thanks for adding this! Sorry it took so long to get it merged.

@newAM newAM added this pull request to the merge queue Dec 20, 2023
Merged via the queue into rust-embedded:main with commit c0649c4 Dec 20, 2023
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants