-
Notifications
You must be signed in to change notification settings - Fork 1
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 :vars to options to allow variables to be specified #32
Conversation
test/jq/api_test.clj
Outdated
(let [script "[$var1, $var2]" | ||
processor-fn (jq/flexible-processor script {:output :string | ||
:vars {:var1 "\"hello\"" | ||
"var2" (.textNode JsonNodeFactory/instance "world")}}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would be a use case for passing raw JsonNode object?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thinking boiled down to "we support JsonNode as an output type, so supporting it as an input type as well without requiring serialization/deserialization supports symmetry". I'm not particularly tied to supporting the case immediately, and would be happy to drop the related code if you prefer.
Mind, I'd be quite a bit happier if in some future version (3.0?) the API stopped accepting strings containing JSON at all, and only accepted JsonNode objects or otherwise objects complying with a "convertable-to-JsonNode" protocol as input. Requiring users to encode data (that may come from Clojure-native types or other formats that weren't originally JSON) into a textual format strikes me as quite contrary to the efforts to build well-optimized code that are evidenced in the effort made around type hinting &c; and it creates ambiguity around "is this a literal string itself, or a string that needs to put through a JSON decoder?"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, symmetry is a good point. If JsonNode are supported in :vars
then an output of a JQ script can be passed 💡
The first version of the lib was really just given two strings (jq script, and JSON string) to return a string back.
Then in another project, I've exposed the jq scripting via this library where multiple scripts can be provided and executed in sequence on the same bit of data. While it worked the encode-decode between JsonNode and string was just plain CPU burning, so the option to return JsonNode which can be immediately passed to another script was added. And here we are.
For future versions, this "convertible-to-JsonNode" issue very high in priority and right next to the proper handling of the stream of output values.
Is 15f683d more like what you had in mind? |
Yes, looks great! |
Because this requires
string->json-node
to be defined beforenew-scope
, the function gets moved down a bit.