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

Fix stderr to output string with no decoration (fix #2063) #2751

Merged
merged 1 commit into from
Jul 23, 2023

Conversation

itchyny
Copy link
Contributor

@itchyny itchyny commented Jul 23, 2023

As written in the manual, the stderr filter should output values in raw and compact mode with no additional decoration. This PR fixes #2063.

@itchyny itchyny added the bug label Jul 23, 2023
@itchyny itchyny force-pushed the fix-stderr-raw-string branch 4 times, most recently from 16cd5e7 to 10e6a61 Compare July 23, 2023 03:19
@@ -232,6 +232,16 @@ grep "Expected string key after '{', not '\\['" $d/err > /dev/null
echo '{"x":"y",["a","b"]}' | $JQ --stream > /dev/null 2> $d/err || true
grep "Expected string key after ',' in object, not '\\['" $d/err > /dev/null

# debug, stderr
$VALGRIND $Q $JQ -n '"test", {} | debug, stderr' >/dev/null
Copy link
Member

@wader wader Jul 23, 2023

Choose a reason for hiding this comment

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

Should output be compared somehow here?

Copy link
Member

Choose a reason for hiding this comment

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

Unrelated thought: I wonder if i would be nicer if we replaced $VALGRIND $Q $JQ by having jq in path be a wrapper script that does exec valgrind -q /path/to/real/jq/ ...? or do we don't want to not run via valgrind in some cases when it's enabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I put this line just for valgrind (and I actually found a leak in the first commit), and I heard Nico said we don't care about the output of debug/0 somewhere. I strongly hope that JQ takes care of valgrind automatically but there might some difficulties in testing the standard error output.

Copy link
Member

@wader wader Jul 23, 2023

Choose a reason for hiding this comment

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

Aha valgrind can messes up stdout/err? by "hope that JQ takes care" do you mean it would be nice if $JQ pointed to some path that runs jq with valgrind?

Yes think i read that comment from @nicowilliams also, but even if we make no guarantees about stableness maybe it can make sense to test debug output so that we would notice if it changes by accident? maybe we already do somewhere?

Copy link
Contributor

Choose a reason for hiding this comment

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

debug/0 should do something with its output. We could have a command-line option to send that to some file, or /dev/null/NUL, and debug/0 can decorate its output or not (but it helps that it does).

I don't really care about the mechanics of how valgrind or other memory debuggers get invoked, as long as a) such debuggers can be optional, b) they can be turned off (valgrind slows dev cycles). If the current thing makes it easy to forget to use valgrind, sure, let's fix it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, if in --run-tests we set a stderr callback, we could have syntax in .test files for checking stderr's output, though I don't think we need that given that a test here in shtest is just as well, and we only need stderr to test it.

@itchyny itchyny added this to the 1.7 release milestone Jul 23, 2023
@nicowilliams nicowilliams merged commit 98f709d into jqlang:master Jul 23, 2023
27 checks passed
@nicowilliams
Copy link
Contributor

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

stderr doesn't print raw
3 participants