-
Notifications
You must be signed in to change notification settings - Fork 70
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 default read offsets for Node Buffer compatibility #99
Conversation
This PR adds an optional offset to the `readXX` methods to create compatibility with the Node Buffer API https://nodejs.org/api/buffer.html#buffer_buf_readbigint64be_offset
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you please add a unit test? Thanks!
@mcollina Hey ok I added some tests. Please check out how I handled the floats, I kind of feel like this is a better test anyway. Rather than testing against hard-coded hex values, we can test equivalence with the output value of a real Node Buffer for the same input. If you agree that this is a better test, I'm happy to update the rest of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, I don't think this is a breaking change in any way, so just a semver-minor?
@rvagg agree, just minor or even patch Did you want me to update the |
This PR adds an optional offset to the
readXX
methods to create compatibility with the Node Buffer API https://nodejs.org/api/buffer.html#buffer_buf_readbigint64be_offset