-
-
Notifications
You must be signed in to change notification settings - Fork 616
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 --nul-output|-0
flag to separate element with NUL character
#1550
Conversation
e5785f0
to
35b92b4
Compare
Hi @vaab - thanks for the contribution :) I'll need to find some time to read through this and the relevant links |
@vaab ok finally got around to reading the jq discussion and looking over the code. I think it does make sense to add this to Looking forward to getting this in! |
acceptance_tests/nul-separator.sh
Outdated
} | ||
} | ||
|
||
wyq() { |
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.
whats the w
stand for here?
acceptance_tests/nul-separator.sh
Outdated
EOL | ||
|
||
res="" | ||
while read-0 a || ! ret="$a"; do |
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.
this lines are black magic to me :)
Many thanks for your interest, and your feedback make all perfect sense to me. I'll correct all this in the upcoming days and add some unit tests. |
ping! |
So sorry, had several things on my plate to deal with and I was not this good at organizing my time. I hope to finish this this week. I'm working on it right now. Thanks for your kind reminder. |
This is to ensure solid parsing of complex data (with any binary content except NUL chars) by separating the `yq` root collection member's output with NUL char. As a safe-guard, an error will be cast if trying to use NUL character with content that contains itself NUL characters inside.
35b92b4
to
6cc43c8
Compare
The code was rebased, your comments were taken into account (I have heavily commented and changed for the better some code on the bash part), and added some unit test as promised. Tell me your feelings. I'm sorry that it might require you to step a little in the murky waters of Thank you very much for your review and sorry for the delay. |
Thanks @vaab - very well commented bash :) |
many thanks for your time and for |
This option is provided to ensure solid parsing of complex data (with any binary content except NUL chars) by separating the
yq
root collection member's output with NUL char. As a safe-guard, an error will be cast if trying to use NUL character with content that contains itself NUL characters inside.I'm aware of the possible heated debate on the inclusion of this as it happened over a similar request on jqlang/jq#1271 ... As a heavy bash user, IMHO, NUL separated output is the safest way to treat complex data in bash, and allows to pipe results directly with many other tools without mangling in bash. The previous thread however have raised important concerns about NUL chars that could be appear in the valid output of a query and could potentially mess with unprepared code badly.
This is why I added a check when using
-0
option to fail with an error when the data being separated with NUL is containing a NUL characters itself. In practice, this is nearly never expected and if it happens, I felt I wanted to be in position to fail and warn the user.I did write some acceptance tests and am very welcome to any of your suggestions. I'll write unit test if you feel this PR is worthwhile for you.
Please note that to be sure of NOT triggering the error, the flag
-r=false
with the yaml encoder needs to be added and NUL chars will be encoded in yaml format which doesn't include raw NUL chars. But the better would probably to fail in the calling code (which is made possible with this implementation) when encountering data that is not formatted as you had expected.As a side-note, I'm the author of
shyaml
, and am using it extensively in bash projects, and would be happy to archive my project : it is slow. Alas, without the current PR,yq
is not able to replace current usages ofshyaml
in my case.There are workaround that I'm aware of (mainly from the
jq
thread posted previously) that are too costly in term of efficiency, code readability or simple data solidity in my own opinion.I would totally understand if you are not interested in this addition on your side. The magic of open-source allows me to keep using it on my side. I'm curious to receive your feedback, as these are complex topics.
This project is probably my first time fiddling with Go language and project, so I probably did some newcomer's mistake. I'll be happy to correct them if you can stress them out for me.
Many thanks for writing
yq
, that's a really awesome project.