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 token position and query json marshal support #123

Closed
wants to merge 1 commit into from

Conversation

wader
Copy link
Contributor

@wader wader commented Sep 10, 2021

Hello!

I've been writing a lot of jq for another project lately so I started looking into making it more comfortable. The result is jq-lsp and vscode-jq. They are both very rough and early in development but i've used them daily for the last weeks without much problems.

This PR is more of a proof-of-concept for the features needed by jq-lsp to implemented the language server. There are probably nicer or better ways for adding token position support or how to allow marshalling of queries.

An issue i stumbled upon when writing a server loop in jq is the issue talked about in #86. For what i can understand it seems like TCO work as it should (it uses def loop(f): def _loop: f | _loop; _loop as loop) but i suspect that function variables or arguments somehow are not poped or reclaimed.

Hope you like it and let me know what you think.

@wader
Copy link
Contributor Author

wader commented Sep 10, 2021

Seems like go run _tools/print_builtin.go is failing, probably because the token positions are different on the values used with DeepEqual. But the other tests should be passing.

@itchyny
Copy link
Owner

itchyny commented Sep 10, 2021

Talking of keeping the token positions, the syntax tree should hold the token position of all the tokens, including keywords, parentheses, operators, commas, semicolons, and comments, so that printing without changing the syntax tree should not change the code at all. I have seen such syntax tree with a Haskell library, which as JSAnnot as the positions for all tokens. And formatters, maybe jqfmt, would be implemented by traversing the syntax tree to modify the positions and then just printing. However based on my experience of performance improvements so far, reducing allocations is critical in Golang, and having the position in the syntax tree in gojq looks just useless in most use cases. So my opinion is that gojq.Query should not have token position. But this is open source, and anyone can create some different parsing (and formatting) library based on this project, and maybe it has un-positioning function to convert to gojq.Query for execution.

@itchyny itchyny closed this Sep 10, 2021
@wader
Copy link
Contributor Author

wader commented Sep 11, 2021

That make sense and thanks for the reference to how the haskell javascript parser does it. I did try to see if i could get comments and whitespace into gojq:s parser but it seemed hard. And i think your correct, a language server and formatter should use different parser and syntax tree for multiple reason, as you said about allocation and also such parser should probably be more forgiving and should be able to recover and handle multiple errors. I've noticed now with gojq:s parser that auto completion sometimes is not so useful as while you type the program is not valid, ex: you start typing a variable $ or want to insert some filter so the | is not added util last.

But writing such a parser is probably quite a lot of work so will see if i have the time to look into it. For now i think gojq:s parser works quite well and it has increased my productivity quite a lot i think. Maybe it would make sense to extract it and add it to jq-lsp directly if more modifications is done?

Also now i'm not sure if it was such a great idea to implement large parts of the lsp server in jq itself, lack of type checking is quite frustrating at times, but was a fun challenged. And once again i'm impressed by jq:s expressiveness.

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