Skip to content

add multi result support of json.get #408

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

Merged
merged 3 commits into from
Jun 30, 2021
Merged

Conversation

MeirShpilraien
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@oshadmi oshadmi left a comment

Choose a reason for hiding this comment

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

👌🏼 Few questions

Comment on lines +836 to +837
env.expect("JSON.GET", "k", '$.*[0,2]').equal(['1', '3', '"c"', '"e"'])

Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. What would be the result of the same GET command if the JSON would initially be
    {"a":[1,2,3],"b":["c", "d", "e"],"c":["k"]}
    (the third element would be an array but with only a single element)
    Should it be ['1', '3', '"c"', '"e"', "k"] ?
  2. Do we want to increase the coverage of multi-paths (more wildcards, and unions, also unions by name, not only by indices, and array slices and recursive descents)
  3. Do we want to also test the legacy mode? Some sanity test?
    e.g., env.expect("JSON.GET", "k", '.[*]').equal('1')

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. yep, this is the json path specification, you can use condition to filter out this result if you want (base on the array size for example)
  2. Yes, but not as part of this PR, this PR purpose to give initial support.
  3. Good point, will add it.

Copy link
Collaborator

@oshadmi oshadmi left a comment

Choose a reason for hiding this comment

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

👍🏼

@MeirShpilraien MeirShpilraien merged commit 9e9eb74 into master Jun 30, 2021
@MeirShpilraien MeirShpilraien deleted the multi_res_on_json_get branch June 30, 2021 07:04
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