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

History: Read change positions at once #2542

Merged
merged 1 commit into from
Jan 25, 2024
Merged

Conversation

SirYwell
Copy link
Member

@SirYwell SirYwell commented Jan 7, 2024

Overview

Description

As a cleanup for v3, I'd like to go from the 3 methods to read a location to one. The current design is flawed; all implementations actually read the whole position in the readX method, requiring the methods to be called in the correct order.
By instead only using one method, we can simplify the code and make it less error-prone. It would also be possible to avoid the byte[] buffers in future this way.

I also marked some classes I touched as internal. They aren't currently, but I don't think they are useful to other developers at all.

Due to the (theoretically) breaking changes and the low priority, this targets v3.

Submitter Checklist

Preview Give feedback

@SirYwell SirYwell requested a review from a team as a code owner January 7, 2024 17:42
@github-actions github-actions bot added the Feature This PR adds a new feature label Jan 7, 2024
@NotMyFault NotMyFault requested a review from a team January 15, 2024 03:50
@SirYwell SirYwell merged commit a31f2f3 into v3 Jan 25, 2024
7 checks passed
@SirYwell SirYwell deleted the feature/v3/history-pos-delegate branch January 25, 2024 08:31
SirYwell added a commit that referenced this pull request Aug 22, 2024
SirYwell added a commit that referenced this pull request Oct 26, 2024
SirYwell added a commit that referenced this pull request Oct 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature This PR adds a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants