Skip to content

Running wsjq#10

Merged
wader merged 12 commits intowader:masterfrom
thaliaarchi:running-wsjq
Sep 8, 2023
Merged

Running wsjq#10
wader merged 12 commits intowader:masterfrom
thaliaarchi:running-wsjq

Conversation

@thaliaarchi
Copy link
Collaborator

This makes the minimal changes needed to run wsjq, at least with count.ws. To test it with wsjq, check out my jqjq-compat branch and run test_jqjq.sh.

I added an intrinsic for test/2, copied the builtins for indices/1, index/1, rindex/1, match/1, test/1, capture/2, and capture/1 from builtin.jq, and reimplemented the _strindices intrinsic as a jq builtin.

I'm not entirely certain what your metric is for your separation between adding a builtin to _builtins_src or to the match over the filter signature. It seems like you prefer deferring to jq for numeric filters and adding most others as builtins, while some like to-/fromjson have their own definitions. _builtins_src doesn't match the source of builtin.jq and appears to be manually edited. Any of the builtins I added would certainly be faster as intrinsics deferring to jq's implementations, but I put them in builtins to follow what looks like your pattern.

Let me know if you'd like any changes.

@wader
Copy link
Owner

wader commented Sep 7, 2023

Wow! nice. Will have a look later, i'm a bit out and about

So there was no issue with "end" being parsed as a function?

There are no strict rules really about intrinsic or anything, but i have tried to reimplement instead of "passthru" as much as possible when "resonable"... not that resonable makes any sense for this projects 😬

@wader
Copy link
Owner

wader commented Sep 7, 2023

Also feel free to add some test to jqjq.test. make test will run the tests with jq and jqjq

@wader
Copy link
Owner

wader commented Sep 7, 2023

If not too much of hack to run maybe can add something to README.md also?

@thaliaarchi
Copy link
Collaborator Author

About end, it turns out I was invoking jqjq wrong. It parses fine.

I can add tests.

I'd rather not link to wsjq from here and say it's officially supported, until there's a better experience running wsjq. Particularly, supporting the flags for raw input and raw output would make a big difference. I could work around --arg/--argfile and library loading with -L by making a separate entrypoint for jqjq, but it would be nice if those were supported. Those would probably need special handling by your bash entrypoint, though. String interpolation sounds tricker and, ideally, I'd rather not maintain a jqjq-compat branch just for desugaring interpolation, but if that's not feasible to implement, I can keep it around. I think raw input and output would be the minimum I'd want, then we can say wsjq on jqjq is officially supported.

@thaliaarchi
Copy link
Collaborator Author

FYI, two already-existing tests fail for both jqjq and jq. jqjq uses else-less if, so it wouldn't have been working with 1.6 and may be something introduced later in the 1.7 cycle or a faulty test.

$ make test-jq
…
*** Expected 3.3333333333333335, but got 3.3333333333333335 for test at line number 335: . / 3
…
*** Insufficient results for test at line number 613: [error, error(null)]
…

$ make test-jqjq
…
DIFF: 62 (line 333): [10 | . / 3] -> [3.3333333333333335]
  Expected: [3.3333333333333335]
    Actual: [3.3333333333333335]
…
ERROR: 117 (line 611): [null | [error, error(null)]] -> [[]]
  null
…

@wader
Copy link
Owner

wader commented Sep 7, 2023

FYI, two already-existing tests fail for both jqjq and jq. jqjq uses else-less if, so it wouldn't have been working with 1.6 and may be something introduced later in the 1.7 cycle or a faulty test.

You mean there are jqjq.test using elseless if? they are guarded by SKIP_BY (but have been enabled now)

For jqjq.jq i've staye away from else-less if, but maybe i messed up somewhere? ... maybe time to allow it?

$ make test-jq
…
*** Expected 3.3333333333333335, but got 3.3333333333333335 for test at line number 335: . / 3
…
*** Insufficient results for test at line number 613: [error, error(null)]
…

$ make test-jqjq
…
DIFF: 62 (line 333): [10 | . / 3] -> [3.3333333333333335]
  Expected: [3.3333333333333335]
    Actual: [3.3333333333333335]
…
ERROR: 117 (line 611): [null | [error, error(null)]] -> [[]]
  null
…

Should be fixed by 157add8

@wader
Copy link
Owner

wader commented Sep 7, 2023

About end, it turns out I was invoking jqjq wrong. It parses fine.

👍

I can add tests.

👍

I'd rather not link to wsjq from here and say it's officially supported, until there's a better experience running wsjq.

No worries, sounds good.

Particularly, supporting the flags for raw input and raw output would make a big difference.

Ideally i think jqjq should always do raw input/output and take care of json parsing encoding itself. but i guess there are some e tricky things like support reading multiple json values. need to have fromjson variant that can return "rest" etc?

I could work around --arg/--argfile and library loading with -L by making a separate entrypoint for jqjq, but it would be nice if those were supported. Those would probably need special handling by your bash entrypoint, though.

I've kind of out of idea how -L /include could be supported as jq has no jq IO functions. Maybe one way could be to support by having some extra arguments to pre-load files into some global state?

String interpolation sounds tricker and, ideally, I'd rather not maintain a jqjq-compat branch just for desugaring interpolation, but if that's not feasible to implement, I can keep it around. I think raw input and output would be the minimum I'd want, then we can say wsjq on jqjq is officially supported.

Yeap it seems a bit tricky to do. If i remember correctly the lexer probably have to become quite a bit more complicated and keep some state around? but maybe there are better ways of doing it. I'll think about it.

Let's see how it goes, work on jqjq is very curiosity/motivational-driven 😄

jqjq.jq Outdated
def capture($val): ($val|type) as $vt | if $vt == \"string\" then capture($val; null)
elif $vt == \"array\" and ($val | length) > 1 then capture($val[0]; $val[1])
elif $vt == \"array\" and ($val | length) > 0 then capture($val[0]; null)
else error( $vt + \" not a string or array\") end;
Copy link
Owner

Choose a reason for hiding this comment

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

Would be nice to make the non-trivial-functions look more like the rest of jqjq

def match($val):
  ( ($val | type) as $vt
  | if $vt == "string" then match($val; null)
    elif $vt == "array" and ($val | length) > 1 then match($val[0]; $val[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.

I've reformatted the new builtins. I'm unsure about capture/2, since that's the only occurrence in jqjq with a reduce with a complex as-binding.

@wader
Copy link
Owner

wader commented Sep 7, 2023

Looks good! just had some minor comments

@wader
Copy link
Owner

wader commented Sep 8, 2023

LGTM, merge?

@wader
Copy link
Owner

wader commented Sep 8, 2023

btw i sometimes run ./jqjq --run-tests < ../jq-repo/tests/jq.test to see how jqjq does with jq's own tests

@thaliaarchi
Copy link
Collaborator Author

thaliaarchi commented Sep 8, 2023

I wrote a better runner, wsjqjq, in a (hopefully temporary) wsjq-compat branch in wsjq, and things are working quite nicely. With it easier to run, I added support for the remaining builtins that wsjq needed. Of the new ones since the last review, the only one I wrote myself was split/1 (which has tests); the others are from builtin.jq.

In the wsjqjq runner, I figured out a workaround, where the output is piped to another jq instance, which strips the JSON syntax with --join-output. jq buffers output when piped, which required me to add --unbuffered to the jq invocation for jqjq, so I had to inline that. Alternatively, it would be nice to have some sort of way to forward args to the jq invocation here.

@thaliaarchi
Copy link
Collaborator Author

I figure a better CLI, that allows forwarding flags like --unbuffered, could be implemented if --raw-input/--raw-output/--join-output become possible.

I noted wsjq support in your README and mine, since the rough edges have been smoothed over by patches here and hacks on my end for what's left. I'd say we're good to merge!

@wader
Copy link
Owner

wader commented Sep 8, 2023

Nice work! 🥳 the "Manually resolve imports." is quite a hack 😄

Yeap let's merge this and we can continue in new PRs to smooth things even more

Big thanks!

@wader wader merged commit a3b5661 into wader:master Sep 8, 2023
@wader
Copy link
Owner

wader commented Sep 8, 2023

Screenshot 2023-09-08 at 15 24 20

so close... long time goal is to get to 99%

@thaliaarchi
Copy link
Collaborator Author

Just a few more builtins should get you to 99%, … and a more fleshed out CLI would set you back haha

@wader
Copy link
Owner

wader commented Sep 8, 2023

Yeap is hard balance! :)

@thaliaarchi thaliaarchi deleted the running-wsjq branch September 12, 2023 08:19
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