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 query support #54

Closed
wants to merge 2 commits into from
Closed

Add query support #54

wants to merge 2 commits into from

Conversation

kulikala
Copy link

Parson already has dotget and dotset functions which I admit are minimal and sufficient as API.
Even so, many of JSON will contain both objects and arrays, in a way it'll be easier to have the accessing syntax to traverse JSON handling both objects and arrays together just like JavaScript.

value = json_value_query_string(root_value, ".list[0].data.prop");
value = json_value_query_string(root_value, ".list[0][data].prop");

@kgabis kgabis self-assigned this Sep 13, 2016
@kgabis
Copy link
Owner

kgabis commented Sep 13, 2016

Hi @kulikala,
I need to think about this feature first. It does seem reasonable but I'd rather not rush into adding anything to the API without thinking of all the pros/cons first.

Regards,
Krzysztof

@w32zhong
Copy link
Contributor

w32zhong commented Sep 13, 2016

This feature looks desirable, if we can test this feature thoroughly and make sure it does not introduce bug into old APIs. After all, the ease of use directly contributes to the popularity of an open source project.

@vovanec
Copy link
Contributor

vovanec commented Sep 13, 2016

+1

@kulikala
Copy link
Author

kulikala commented Sep 14, 2016

Dear @kgabis,

When you call json_value_query_value function, it just returns a pointer to single JSON_Value instance in the given JSON tree.
I guess the point of this argument is lying here.

Here are the pros and cons I can think of:

pros

  • Retrieving a value from a JSON object/array become requiring less code
  • json_value_query_value does not allocate new memory,
    so json_value_query_value is fast, one pass run, and
    you can cleanup all code with single json_value_free call
  • The query is similar to JavaScript accessor and intuitive
  • The adding codes change anything of existing codes and APIs

cons

  • json_value_query_* brothers only accept pointer of JSON_Value;
    this approach may conflicts in a way with API's main approach, json_object_* sisters
  • The query is not strict at all: ".", "..", "...", "[]", ".[]" all return the root value
  • The query ".." looks like the descendant operator of E4X proposal but it's not
  • The query looks like JSONPath but it's not
  • Maybe object keys wrapped with brackets [] might be with single quotes '':
    .object[key][1] might be .object['key'][1]
  • The query would create another JSON_Value instance, should return multiple values with JSON_Array

Fix the argument name of the implementation so that it fits the
declaration.
@kgabis
Copy link
Owner

kgabis commented Sep 25, 2016

I'm still not convinced this should end up in the library. The only use case I can think of when array notation will be useful is when you know exactly how many elements an array has which I assume is not that common. It might be nice to have this functionality in dotget/dotset functions (although I like name query a lot more, but it's too late to change that) however I'd rather not add a whole new set of functions just for that. It might confuse library users because there will be 2 ways to accomplish a similar goal. The only downside of using dotget/dotset functions is that they must be called on objects so it won't be possible to call them on an array.
The other reason not to merge this pull request is that there is a memory leak here and here and I don't like the fact that query syntax is not strict.

I've made an issue (not really an issue though) for this feature at #57.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants