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

JSON parsing and printing for messages #68

Merged
merged 2 commits into from Mar 19, 2020

Conversation

@siddharthab
Copy link
Contributor

@siddharthab siddharthab commented Mar 17, 2020

Resolves #61.

@eddelbuettel
Copy link
Owner

@eddelbuettel eddelbuettel commented Mar 17, 2020

Nice work -- the PR looks very clean. I will take another, closer, look later.

R/read.R Outdated Show resolved Hide resolved
@eddelbuettel
Copy link
Owner

@eddelbuettel eddelbuettel commented Mar 19, 2020

I made a minor local commit to get rid of two warnings under R CMD check but as I can't push that back to you I will just merge and proceed. Hope that is ok.

@eddelbuettel eddelbuettel merged commit 6099525 into eddelbuettel:master Mar 19, 2020
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@siddharthab
Copy link
Contributor Author

@siddharthab siddharthab commented Mar 19, 2020

Thank you for the quick review.

@eddelbuettel
Copy link
Owner

@eddelbuettel eddelbuettel commented Mar 19, 2020

My pleasure -- thanks for the PR. I tend to get to get to things sooner, but I had a distration from a Rcpp release which may need a follow-up given an issue on macOS we did not catch / could not catch for lack of reverse-depends checking infrastructure on macOS :-/ Plus an issue with BioConductor which I may have to add to the test matrix...

I presume you looked at my follow-up commit and have no issues? I added you to ChangeLog too.

@siddharthab
Copy link
Contributor Author

@siddharthab siddharthab commented Mar 19, 2020

Thanks! The changes in the follow-up commit look good.

The next thing I would like to add when I have the time is to return a helpful message when people mistype field names instead of segfaulting.

@eddelbuettel
Copy link
Owner

@eddelbuettel eddelbuettel commented Mar 19, 2020

Having slept over it, I think I will restore the small 'if open' change.

Catching such a segfault would be awesome! I'll release this as an interim version with JSON support.

@eddelbuettel
Copy link
Owner

@eddelbuettel eddelbuettel commented Mar 19, 2020

I just looked at it in more detail. Removing the condition in the readASCII case literally the last thing I did -- it must have come up when I switched the tests from RUnit to tinytest revealing some more verbosity. Hence, let's keep it as you amended it. As in readASCII. If it barfs we can adjust.

@siddharthab
Copy link
Contributor Author

@siddharthab siddharthab commented Mar 19, 2020

Sounds good.

@siddharthab siddharthab deleted the grailbio-external:json branch Mar 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants
You can’t perform that action at this time.