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

toJSON(auto_unbox=TRUE, force=TRUE) should not unbox objects wrapped in I() #78

Closed
wch opened this issue Mar 16, 2015 · 8 comments · Fixed by #79
Closed

toJSON(auto_unbox=TRUE, force=TRUE) should not unbox objects wrapped in I() #78

wch opened this issue Mar 16, 2015 · 8 comments · Fixed by #79

Comments

@wch
Copy link
Contributor

@wch wch commented Mar 16, 2015

For compatibility with RJSONIO, objects wrapped in I() shouldn't be unboxed, even when auto_unbox=TRUE.

For example:

RJSONIO:::toJSON(list(x = I("text")))
# [1] "{\n \"x\": [ \"text\" ] \n}"

jsonlite:::toJSON(list(x = I("text")), auto_unbox = TRUE, force = TRUE)
# {"x":"text"} 

@jeroenooms would this just involve writing a new method for AsIs objects?

This is related to timelyportfolio/parcoords#6.

@yihui

This comment has been minimized.

Copy link

@yihui yihui commented Mar 16, 2015

+1

I like the I() notation to box scalars in RJSONIO. It is very easy to opt-in, since I() is a one-letter function in base R. By comparison, it is relatively expensive to opt-out boxing in jsonlite when auto_unbox = FALSE: one need to type jsonlite::unbox(), and this can be cumbersome when it has to be done multiple times, e.g.

datatable(iris, options = list(order = list(
  list(jsonlite::unbox(1), jsonlite::unbox('asc')),
  list(jsonlite::unbox(2), jsonlite::unbox('desc'))
)))

On the other hand, when auto_unbox = TRUE, currently there is no way to opt-in boxing in jsonlite if we do want to box certain scalars but not all of them. It will be nice if the class AsIs is supported.

@jeroen

This comment has been minimized.

Copy link
Owner

@jeroen jeroen commented Mar 17, 2015

The only thing that makes me a little nervous is that we introduce undefined behavior for I() objects that are not scalars.

@wch

This comment has been minimized.

Copy link
Contributor Author

@wch wch commented Mar 17, 2015

Can you give an example? In my mind it seems pretty clear: I() only affects the behavior if the content is a length-1 scalar (atomic vector).

For things like I(list(3)) or I(1:3), the content of I() is not a length-1 atomic vector, and therefore the I() has no effect. I tried to get at that in the tests in my PR.

@wch

This comment has been minimized.

Copy link
Contributor Author

@wch wch commented Mar 17, 2015

Also, if you'd prefer an option for this behavior, I'm amenable to that as well -- although to me it doesn't seem necessary since auto_unbox, I(), and a scalar would be a rare combination to see by chance.

@wch

This comment has been minimized.

Copy link
Contributor Author

@wch wch commented Mar 17, 2015

Oh, I just realized that you have a class named scalar in jsonlite. Is that what you're referring to?

@jeroen

This comment has been minimized.

Copy link
Owner

@jeroen jeroen commented Mar 17, 2015

Sorry I was confused. I meant the case of I(1:3) where the user can use I() when it is not appropriate. But it is harmless.

@jeroen jeroen closed this in #79 Mar 17, 2015
@yihui

This comment has been minimized.

Copy link

@yihui yihui commented Mar 17, 2015

Exactly. It is harmless.

@vspinu

This comment has been minimized.

Copy link

@vspinu vspinu commented Aug 16, 2015

Big thanks for this, but it should be documented. Maybe in unbox help page.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.