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

fix: getNumber varint decode bug #65

Merged
merged 1 commit into from
Sep 7, 2020
Merged

fix: getNumber varint decode bug #65

merged 1 commit into from
Sep 7, 2020

Conversation

JonasKruckenberg
Copy link
Contributor

This hopefully fixes #50 by removing the call to numberToUint8Array() in varintUint8ArrayDecode.
It returned a wrongly encoded Uint8Array that had 1 as it's first byte, so all codec above 256 could not be resolved.

Copy link
Member

@vmx vmx 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 your contribution, fixing this very long lasting bug. Please see my comment for even less code.

src/index.js Outdated Show resolved Hide resolved
test/multicodec.spec.js Show resolved Hide resolved
@JonasKruckenberg
Copy link
Contributor Author

Should all be resolved.

@hacdias
Copy link
Member

hacdias commented Sep 6, 2020

Hey @JonasKruckenberg! It LGTM. Could you just fix the linting issues as well as squashing the commits into one using the conventional message format so we can have CI running?

@vmx
Copy link
Member

vmx commented Sep 7, 2020

You can run linting locally via npm run lint.

@JonasKruckenberg
Copy link
Contributor Author

Alright, I had to do more rebase magic that I expected, but now things should be nice and tidy. The failing coverage test is nothing I can fix right? Because it seems like it's complaining just because I removed code.

@vmx
Copy link
Member

vmx commented Sep 7, 2020

@JonasKruckenberg Thanks a lot of the hard work and reworking everything so that it conforms to our CI stuff (yes you can ignore that codecov error).

In case you still feel like polishing, your commit message has typo: "deocde" -> "decode".

This fixes #50 by removing the varintUint8ArrayDecode function and using varint.decode directly instead.
@JonasKruckenberg
Copy link
Contributor Author

Thanks hahaha, I didn't notice that, I also spelled it wrong twice in the same commit.
Anyway, it was a pleasure!
Cheers

@hacdias hacdias changed the title Fix getNumber fix: getNumber varint decode bug Sep 7, 2020
@hacdias hacdias merged commit ce93ceb into multiformats:master Sep 7, 2020
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.

getNumber() doesn't work correctly
3 participants