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

Events::oldest_event_count and Events::oldest_id don't make the difference to each other clear enough #15617

Open
urben1680 opened this issue Oct 3, 2024 · 2 comments
Labels
C-Docs An addition or correction to our documentation S-Needs-Triage This issue needs to be labelled

Comments

@urben1680
Copy link
Contributor

urben1680 commented Oct 3, 2024

I am not certain if this is a docs issue, but I found this by looking at the source, not by encountering some bug. That is why for now I focused on how the API appears while there are more issues.

The methods Events::oldest_event_count and Events::oldest_id seem to do the same thing when looked at the docs or the source.

Also the API itself is confusing. Instead of returning usize it should return an Option<EventId<E>> because no oldest event might be available currently if the sequences events_b and events_a are empty.

As a side note, the implementation of oldest_event_count looks like that:

/// Returns the index of the oldest event stored in the event buffer.
pub fn oldest_event_count(&self) -> usize {
    self.events_a
        .start_event_count
        .min(self.events_b.start_event_count)
}

When it should be clear that self.events_b as the more recent sequence cannot contain the oldest count.

@urben1680 urben1680 added C-Docs An addition or correction to our documentation S-Needs-Triage This issue needs to be labelled labels Oct 3, 2024
@urben1680
Copy link
Contributor Author

Tagging @Aceeri as the author of both methods. 👋

@Aceeri
Copy link
Member

Aceeri commented Oct 3, 2024

Ya oldest_id should just be renamed to oldest_event_count and it's implementation is generally better. Probably just missed this during merging + the reverting of the warning message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Docs An addition or correction to our documentation S-Needs-Triage This issue needs to be labelled
Projects
None yet
Development

No branches or pull requests

2 participants