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

a little too much enthusiasm for unpacking objects without a JSON representation #277

Open
ExpandingMan opened this issue Jan 18, 2019 · 2 comments

Comments

@ExpandingMan
Copy link

This package seems to go pretty far in unpacking objects that don't necessarily have an obvious JSON representation. This can be a pretty cool feature, but it can also lead to some weird unexpected behavior. The example that burned me is

julia> JSON.json(skipmissing([1,missing,3]))
"{\"x\":[1,null,3]}"

This of course happens because skipmissing returns an AbstractVector which refers to the original object and simply skips the missings during iteration, and the field in the skipmissing iterator that refers to the parent array is called x. I think it's safe to say that most people would probably expect any AbstractVector just convert into a JS list corresponding to the AbstractVector itself and not the underlying data.

So, my proposal is that at least for AbstractArray and AbstractDict types the JSON generated should attempt to show the object represented as an actual JS list or dict, rather than showing the underlying data.

Thoughts?

@TotalVerb
Copy link
Collaborator

This seems very reasonable, and is indeed already the behavior, as far as I know. But unfortunately the return value of skipmissing is not an AbstractArray. We can try to turn all iterables to JS lists instead of unpacking them as a struct?

@ExpandingMan
Copy link
Author

Sorry, somehow I missed your response until now.

I had not realized when I opened this issue that skipmissing is not an AbstractArray. Now that I'm thinking about it, I suppose the reason for this is that it does not have a known length.

I'm no longer sure what I think the right solution is. Converting all iterables into JS lists seems a little aggressive, and the best way to determine whether something is with hasmethod(iterate, T) (which is a bit slow, but perhaps not prohibitively so).

I'm wondering if it's perhaps better to throw an error when encountering new structs? That seems far safer than trying to unpack them. As long as JSON.jl converts AbstractVector and AbstractDict correctly, this should only happen on types that don't have an immediately obvious JS interpretation.

When I encountered the error I was sort of carelessly calling JSON.json on stuff that was ultimately going to be read by a REST API. Since that's a pretty common use case, it might be a little better to err on the side of safety for unknown types.

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

No branches or pull requests

2 participants