Skip to content

Fix binary string issue #10

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

Open
wants to merge 23 commits into
base: develop
Choose a base branch
from
Open

Conversation

smittytone
Copy link

See https://electricimp.atlassian.net/browse/CSE-702

Convert blob values to base64 strings

@smittytone
Copy link
Author

How do I see what's failing in the tests?

@betzrhodes
Copy link

How do I see what's failing in the tests?

you can view the test status at https://cse-ci.electricimp.com, log-in as a guest, click on the project and look for the build log for the pull request that is failing.

@smittytone
Copy link
Author

@betzrhodes I've come back to this -- left it too long, really -- and fixed the breaking tests. I'd forgotten to update them for the new way (correct as per JSON spec.) of doing blobs. As a result, I've bumped this update to 3.0.0 since it's a breaking change.

This could therefore do with a review, when you get a moment.

Copy link

@betzrhodes betzrhodes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are going to make a breaking change we should also update the file name to lib.nut.

README.md Outdated

This library can be used to encode Squirrel data structures into JSON.

**To add this library to your project, add** `#require "JSONEncoder.class.nut:2.0.0"` **to the top of your code.**
**To include this library in your project, add** `#require "JSONEncoder.class.nut:2.0.1"` **at the top of your code.**

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this version number doesn't match

@betzrhodes
Copy link

betzrhodes commented Oct 18, 2019

Something to think about. This update leaves our JSON libraries out of sync with each other -

If I encode a blob with the encoder and then parse it with the parser I will not get the result I would expect:

jsonString <- JSONEncoder.encode(b);
result <- JSONParser.parse(jsonString);

server.log(crypto.equals(result, b));

While this approach mitigates the errors I'm not sure this is the best direction for the library to head in.

Copy link

@betzrhodes betzrhodes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels much better. Will need to update JSONParser as well.

On another note: We should probably take the opportunity to update the name of this library.
Change the name to "JSONEncoder.lib.nut" and bump the version to v3.0.0.

@@ -1,23 +1,23 @@
#require "JSONEncoder.class.nut:1.0.0"
#require "JSONEncoder.class.nut:3.0.0"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrong version number

local uniStr1 = "💾❤️😎🎸";
local uniStr2 = "\xF0\x9F\x98\x9C";

local j = JSONEncoder.encode({"binary_data":b, "uni_strings": [{"uni_string_one":uniStr}, {"uni_string_two":uniStr2}]});

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo - ERROR: the index 'uniStr' does not exist

local ch2 = (str[++i] & 0xFF);
res += format("%c%c", ch1, ch2);
} else {
//throw "Insufficient data for 2-byte Unicode";

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove - throw error

local ch3 = (str[++i] & 0xFF);
res += format("%c%c%c", ch1, ch2, ch3);
} else {
//throw "Insufficient data for 3-byte Unicode";

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same - remove throw error

local ch4 = (str[++i] & 0xFF);
res += format("%c%c%c%c", ch1, ch2, ch3, ch4);
} else {
//throw "Insufficient data for 4-byte Unicode";

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same - remove throw error

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.

2 participants