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

Improve support for integer values #147

Merged
merged 2 commits into from
Feb 16, 2023

Conversation

pbricout
Copy link
Contributor

@pbricout pbricout commented Feb 15, 2023

The documentation does not specify that the timeStamp values must be an integer.
When the values are not an integer the appendNumber function silently omit to write the value and creates an invalid encoded buffer.

  • update the documentation to specify that the timeStamp must be a positive integer
  • update the appendNumber function to throw a RangeError when the given value is not an integer instead of creating an invalid buffer

fixes #146

If the given argument is not an integer, the value will not
be encoded and the overall encoding becomes invalid because
a value is missing.

It is better to throw rather than silently failing.
@pbricout
Copy link
Contributor Author

It is arguable whether we want to automatically truncate the values like in some other cases, but I think it is better to notify the user that something is wrong rather than automagically trying to fix the value.

Maybe we should also update the documentation to specify that failing to meet this requirement will throw.

@pbricout
Copy link
Contributor Author

Hey @Zazama do you mind having a look at this PR? Thanks.

@Zazama
Copy link
Owner

Zazama commented Feb 16, 2023

Sounds reasonable, kind of interesting to learn that there is no normal int type in js. I think the documentation is clear like this

@Zazama Zazama merged commit e73839f into Zazama:master Feb 16, 2023
@pbricout pbricout deleted the improve-support-for-integer-values branch February 16, 2023 21:53
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.

Incorrect timestamps on Syncronized lyrics
2 participants