Skip to content

Merge in upstream binary-parser #18

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

Merged
merged 92 commits into from
Nov 23, 2019
Merged

Conversation

NotBobTheBuilder
Copy link

It looks like the upstream has made a bunch of changes since your original (awesome) PR was filed.

I happened to need some of the additional features (particularly Int64s) so did my best to resolve the conflicts.

Much of this was manual so I have undoubtedly made mistakes, but I hope it's at least helpful!

If you spot any bugs I should fix, let me know. Alternatively if there are other change you want to make, the PR is filed with "allow edits from maintainers enabled".

The main remaining TODOs are commented in the code - additional parsers were added upstream, . pointer and .saveOffset (possibly internal? definitely undocumented). These still need encoders to be implemented, but I haven't done that yet as I'm not sure how.

Hopefully this PR is helpful!

Simran-B and others added 30 commits April 23, 2018 09:34
Was "assert" called "validate" in an older version? Can't find any use or definition of "validate" in the current one.
…nto Simran-B-patch-1

# Conflicts:
#	example/jpg.js
JPG example should use assert, not validate keichi#80
Inject Buffer into the vm context
Fix module not found error when running the examples
Statically declare all parser methods
Refactoring to leverage TS language features
@NotBobTheBuilder
Copy link
Author

There are still a couple of outstanding bugs in the tests from my resolution, so I'll work on resolving those

@NotBobTheBuilder
Copy link
Author

All the tests are now passing! Sorry for the noise getting to this point - I hadn't noticed the tests weren't running at first.

@Ericbla
Copy link
Owner

Ericbla commented Aug 20, 2019

Thanks for this great job.

I know that the PullRequest for BigInt support is now merged in the smart-buffer package, so there is no more reason to not resync with upstream binary-parser.

However, it will require to add additional tests (at least for BigInt encoding) and I will have to switch my mind to move to TypeScript.

I will requires some delay as I've switched to another legacy C++ project.

So don't blame me if it takes sometime for your PR to be merged.

B.R.

@NotBobTheBuilder
Copy link
Author

Awesome! Thanks for the feedback.

No worries about this taking time. I was using this for a week long project and wanted to make available the changes I had made. I don't expect this to get merged any time soon.

Likewise it may be a while before I find the time to add tests but I will get around to doing it in the next few weeks.

Thanks again!

Ericbla added a commit that referenced this pull request Nov 23, 2019
@Ericbla Ericbla merged commit 1f8bdfa into Ericbla:encoder Nov 23, 2019
Ericbla added a commit that referenced this pull request Nov 23, 2019
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.

4 participants