Skip to content

feat: json traversal - #170

Merged
matthias-Q merged 6 commits into
mainfrom
dmf/json
Jun 29, 2023
Merged

feat: json traversal#170
matthias-Q merged 6 commits into
mainfrom
dmf/json

Conversation

@dmfay

@dmfay dmfay commented Jun 29, 2023

Copy link
Copy Markdown
Collaborator

this needs more time than I have for it at the moment but I don't think it'll be too hard to get basic support for #166! element indexing (possibly some common ground with the array[1,2,3] syntax?) and jsonpath are bigger considerations though.

@dmfay
dmfay requested review from DerekStride and matthias-Q June 29, 2023 01:15
@dmfay
dmfay marked this pull request as draft June 29, 2023 01:15
@matthias-Q

Copy link
Copy Markdown
Collaborator

I added the other json operators and updated the test. Actually it looks like this is it. All other json operations are probably handled by invocation

@Gelio

Gelio commented Jun 29, 2023

Copy link
Copy Markdown

Drive-by appreciation comment:

I am honestly amazed by your aptitude and the promptness at fixing the issues I've reported. Stellar project maintenance! I certainly wasn't expecting to see all 3 issues addressed the same day I reported them

@dmfay
dmfay marked this pull request as ready for review June 29, 2023 12:30
@dmfay

dmfay commented Jun 29, 2023

Copy link
Copy Markdown
Collaborator Author

Drive-by appreciation comment:

I am honestly amazed by your aptitude and the promptness at fixing the issues I've reported. Stellar project maintenance! I certainly wasn't expecting to see all 3 issues addressed the same day I reported them

the JSON issue has secretly been bugging me for a while (along with array literals and subscripting, but that's for later) and I've always been too busy with other things & other projects to take a look, and it turns out it just fits right into the expression node now

@matthias-Q matthias-Q left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM. Thanks for fixing the tests.

@matthias-Q matthias-Q changed the title feat (wip): json traversal feat: json traversal Jun 29, 2023
Comment thread test/corpus/json.txt Outdated
Comment on lines +26 to +28
(json_traversal
(json_traversal
(literal))))

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This subtree represents the term "user"->'address' ->> 'city'. It only references a single literal node which means the other two have been hidden.

I've puts together #171 to show an alternative using the binary_expression node that will surface all 3 literals and treat the navigation nodes as anonymous operators.

alternative implementation using binary_expression
@matthias-Q

Copy link
Copy Markdown
Collaborator

We should add the new operators to the highlights.scm (i forgot that with filter yesterday as well ...)

@matthias-Q

Copy link
Copy Markdown
Collaborator

This looks good to me. I am merging it. I am so surprised that this went so smoothly.

(I was hoping that after the recent refactor we could also tackle array and not in, but it seems like they are still difficult to solve)

@matthias-Q
matthias-Q merged commit ab5f935 into main Jun 29, 2023
@dmfay

dmfay commented Jun 29, 2023

Copy link
Copy Markdown
Collaborator Author

This looks good to me. I am merging it. I am so surprised that this went so smoothly.

(I was hoping that after the recent refactor we could also tackle array and not in, but it seems like they are still difficult to solve)

I went looking and we actually got arrays at some point!

@dmfay
dmfay deleted the dmf/json branch June 29, 2023 23:50
@matthias-Q

Copy link
Copy Markdown
Collaborator

@dmfay yes, but not as a function. See #115 Besides, I think #128 is more important :-/

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.

4 participants