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

Add return_truncated decode flag #158

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sargun
Copy link

@sargun sargun commented Nov 22, 2017

No description provided.

Copy link
Owner

@davisp davisp left a comment

Choose a reason for hiding this comment

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

The implementation looks great but I'm wondering what the use case is exactly? Is this to detect where there's broken JSON or something? Also not entirely certain it needs to be an option seeing as we could just make it the default. I've got two or three other breaking changes I'd like to make as well and then finally call it a 1.0 I'd be fine adding this to that list rather than making it an option if that seems more logical.

c_src/decoder.c Outdated
@@ -1035,7 +1039,11 @@ decode_iter(ErlNifEnv* env, int argc, const ERL_NIF_TERM argv[])
}

if(dec_curr(d) != st_done) {
ret = dec_error(d, "truncated_json");
if (d->return_truncated)
Copy link
Owner

Choose a reason for hiding this comment

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

You need to add braces around the bodies of this if/else statement.

@sargun
Copy link
Author

sargun commented Nov 22, 2017

The use case is that I have a protocol which takes JSON over streaming sockets (TCP, websocket). JSON is basically the "framing" format. Because of the properties of the protocol, you can get partial frames. In that case, I know to go ahead and wait for more and add it to the buffer before trying to decode again.

I know that JSX has a streaming API, but I personally find it really difficult to use, especially when my messages are <16KB. A simple decode, append, continue loop works just fine.

@davisp
Copy link
Owner

davisp commented Nov 22, 2017 via email

@sargun
Copy link
Author

sargun commented Nov 24, 2017

Yeah, I'd think so, the only reason I'm returning the extra position is because I wanted it to be as similar to the existing API mechanism as possible. I agree, it doesn't make a lot of sense, but I'd hate to break API backwards compatibility.

@sargun sargun force-pushed the return-on-truncated branch from eac9574 to 66797a9 Compare November 25, 2017 08:56
@sargun
Copy link
Author

sargun commented Nov 25, 2017

Fixed the braces.

@sargun
Copy link
Author

sargun commented Dec 4, 2017

How do you want to proceed?

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