Binary primitive parsers. #264

theqabalist · 2018-06-26T01:38:27Z

This adds a useful array of primitive buffer parsers. Reference #261.

theqabalist · 2018-06-26T01:44:29Z

@wavebeem I need to write all the documentation, but feel free to start perusing the changes.

wavebeem · 2018-06-26T01:53:06Z

test/core/doubleLE.test.js

+"use strict";
+
+describe("doubleLE", function() {
+  it("reads a double, big-endian", function() {


should say little-endian

wavebeem

Looks good so far, just a few thoughts. Thanks for taking a look at this so quickly.

wavebeem · 2018-06-26T02:10:06Z

src/parsimmon.js

+function uintBE(length) {
+  if (length < 0 || length > 6) {
+    throw new Error("uintBE requires length in range [0, 6].");
+  }


Might be worth checking the number is an integer also. And then putting the logic into a helper function for reuse.

typeof length === "number" && Math.floor(length) === length is probably the clearest integer check in JS?

wavebeem · 2018-06-26T02:11:25Z

test/core/floatBE.test.js

+  it("reads a float, big-endian", function() {
+    var b = Buffer.from([1, 2, 3, 4]);
+    var p = Parsimmon.Binary.floatBE;
+    assert.deepEqual(p.tryParse(b), 2.387939260590663e-38);


Believe it or not, this is my lucky number!!

Lol, I got this directly from the node buffer page. I figured if it's good enough for their example, it's good enough for a test case.

wavebeem · 2018-06-26T02:21:37Z

src/parsimmon.js

-  if (length < 0 || length > 6) {
-    throw new Error("uintBE requires length in range [0, 6].");
+  if ((isInteger(length) && length < 0) || length > 6) {
+    throw new Error("uintBE requires integer length in range [0, 6].");


I think the parentheses are in the wrong spot on this line. Also it might be nice to put this entire if condition into a helper function since it's the same in several locations

I'll put this into a helper function. The parens were inserted by the style fixer I think, probably to emphasize the actual associativity, which is wrong, and I am going to fix.

wavebeem

Left a couple suggestions on testing

wavebeem · 2018-06-26T04:07:54Z

src/parsimmon.js

+
+function isInteger(value) {
+  return typeof value === "number" && Math.floor(value) === value;
+}


Fun fact: I just checked the MDN polyfill for isInteger and it also checks isFinite but we won't need that since we're also checking the range

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/isInteger

wavebeem · 2018-06-26T04:24:15Z

test/core/encodedString.test.js

+    var p = Parsimmon.Binary.encodedString("ascii", 11);
+    assert.deepEqual(p.tryParse(b), "hello world");
+  });
+});


I think it would be good to have examples of utf16le, latin1, and utf8 strings with non-ASCII characters also

Maybe a test with ✓, “, ”, and some alphabet characters?

https://nodejs.org/api/buffer.html#buffer_buffers_and_character_encodings

wavebeem · 2018-06-26T04:25:53Z

src/parsimmon.js

+  uintBE: uintBE,
+  uint8BE: uintBE(1),
+  uint16BE: uintBE(2),
+  uint32BE: uintBE(4),


So the error message for uint32BE will say uintBE(4), right?

Yes, that is true. Do you think that's too far removed? I was trying to save some boilerplate.

Honestly it's probably not a big deal. I think just for error messages we can change that later anyway.

wavebeem · 2018-06-26T16:31:43Z

Hah, it looks like Node 4 and Node 5 don't have latin-1 encoding support! Which is why the tests are failing

Given those are both EOL versions I suppose we could just remove them from the travis.yml 🤔

theqabalist · 2018-06-26T19:34:31Z

Do you want to do that or just remove the latin-1 example? I am thinking that if you can show that multiple encoding/decoding works, then it's relatively safe to assume the rest of the pattern will work under other encoders.

theqabalist · 2018-06-26T20:20:11Z

@wavebeem please double check my documentation copy. I tried to look through it myself, but the signed/unsigned le/be 8/16/32 variants make my eyes go crazy. I think I got them all right.

wavebeem · 2018-06-27T05:32:35Z

Hah, I know how that is with docs. I'll try and look at this as soon as I can.

As for Latin-1... I dunno we could remove it but with Node 4/5 no longer supported I think it's fine to drop it from the CI pipeline 🤷‍♂️

wavebeem

I think this looks good all around. I'll try to get this merged and released soon.

Would you mind dropping Node 4/5 from the travis.yml too so we can keep the CI golden?

wavebeem · 2018-07-02T17:50:45Z

src/parsimmon.js

+  uintBE: uintBE,
+  uint8BE: uintBE(1),
+  uint16BE: uintBE(2),
+  uint32BE: uintBE(4),


Honestly it's probably not a big deal. I think just for error messages we can change that later anyway.

coveralls · 2018-07-10T17:47:04Z

Coverage remained the same at 100.0% when pulling 69b1a72 on theqabalist:master into f3699ec on jneen:master.

coveralls · 2018-07-10T17:47:05Z

Coverage remained the same at 100.0% when pulling 69b1a72 on theqabalist:master into f3699ec on jneen:master.

wavebeem · 2018-07-10T18:18:27Z

Thanks! I'm gonna get back to making a binary parser example after merging this and may tag you in that PR if you don't mind.

theqabalist · 2018-07-10T18:21:49Z

Works for me. I'll stay on the lookout.

wavebeem

As I was sitting down to make my binary example I noticed that intLE says uintLE. Would you mind fixing that when you get a chance?

wavebeem · 2018-07-11T04:53:39Z

src/parsimmon.js

+function intLE(length) {
+  assertValidIntegerByteLengthFor("intLE", length);
+
+  return parseBufferFor("uintLE(" + length + ")", length).map(function(buff) {


I just noticed that this says uintLE on the second string by mistake.

I'll just fix this myself idk why I even bothered you about this lol

wavebeem · 2018-07-11T05:20:53Z

Also this is what I've got going for a binary parser example right now: https://gist.github.com/wavebeem/68081d43f1bae5b1c8e1e64abbd03437

wavebeem · 2018-07-13T03:23:25Z

Thanks again for your work on this! I just released 1.12.0 and announced it.

Brandon Keown added 2 commits June 25, 2018 20:35

Binary primitive parsers.

0dfbfc7

Merge from upstream.

4744930

wavebeem reviewed Jun 26, 2018

View reviewed changes

Brandon Keown added 2 commits June 25, 2018 21:04

Fix merge duplication, correct test copy.

7119df2

Style fixes.

ed95007

wavebeem reviewed Jun 26, 2018

View reviewed changes

Add integer check, add copy for integer check.

393eea3

wavebeem reviewed Jun 26, 2018

View reviewed changes

Brandon Keown added 2 commits June 25, 2018 21:24

Revert some problematic changes.

90e4ad1

Move condition into assert helper.

5be5520

wavebeem reviewed Jun 26, 2018

View reviewed changes

Expand encodedString test.

6d6d73e

Documentation for new parser primitives.

57010e3

wavebeem approved these changes Jul 2, 2018

View reviewed changes

Remove node 4/5 from travis.yml.

69b1a72

wavebeem requested changes Jul 11, 2018

View reviewed changes

wavebeem approved these changes Jul 11, 2018

View reviewed changes

wavebeem merged commit 5b81df1 into jneen:master Jul 11, 2018

wavebeem mentioned this pull request Aug 7, 2018

New Parsimmon.Binary primitives #261

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Binary primitive parsers. #264

Binary primitive parsers. #264

theqabalist commented Jun 26, 2018

theqabalist commented Jun 26, 2018

wavebeem Jun 26, 2018

wavebeem left a comment

wavebeem Jun 26, 2018

wavebeem Jun 26, 2018

theqabalist Jun 26, 2018

wavebeem Jun 26, 2018

theqabalist Jun 26, 2018

wavebeem left a comment

wavebeem Jun 26, 2018

wavebeem Jun 26, 2018

wavebeem Jun 26, 2018

theqabalist Jun 26, 2018

wavebeem Jul 2, 2018

wavebeem commented Jun 26, 2018

theqabalist commented Jun 26, 2018

theqabalist commented Jun 26, 2018

wavebeem commented Jun 27, 2018

wavebeem left a comment •

edited

Loading

wavebeem Jul 2, 2018

coveralls commented Jul 10, 2018

coveralls commented Jul 10, 2018 •

edited

Loading

wavebeem commented Jul 10, 2018

theqabalist commented Jul 10, 2018

wavebeem left a comment

wavebeem Jul 11, 2018

wavebeem Jul 11, 2018

wavebeem commented Jul 11, 2018

wavebeem commented Jul 13, 2018

Binary primitive parsers. #264

Binary primitive parsers. #264

Conversation

theqabalist commented Jun 26, 2018

theqabalist commented Jun 26, 2018

Choose a reason for hiding this comment

wavebeem left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wavebeem left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wavebeem commented Jun 26, 2018

theqabalist commented Jun 26, 2018

theqabalist commented Jun 26, 2018

wavebeem commented Jun 27, 2018

wavebeem left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jul 10, 2018

coveralls commented Jul 10, 2018 • edited Loading

wavebeem commented Jul 10, 2018

theqabalist commented Jul 10, 2018

wavebeem left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wavebeem commented Jul 11, 2018

wavebeem commented Jul 13, 2018

wavebeem left a comment •

edited

Loading

coveralls commented Jul 10, 2018 •

edited

Loading