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

do not print to stdout when STREAM=0 #11

Closed
wants to merge 2 commits into from

Conversation

aya
Copy link

@aya aya commented Feb 25, 2019

No description provided.

@step-
Copy link
Owner

step- commented Feb 26, 2019

Hi,

thank you for your pull request. I think you misunderstood the purpose of function apply. It is a stub, simply meant to provide a placeholder for your custom processing when you pass -v STREAM=0 to JSON.awk. It is somewhat misleading that apply prints the JPATHS array but remember, it is a stub. In real-world, embedded use of JSON.awk you application would have function apply do something more meaningful with JPATHS than just printing it out.

The role of STREAM and BRIEF can be better seen by running the following shell commands. But first, edit line 65 to insert 65, into the print argument list, and line 181 to insert 181, likewise. This way you will be able to spot who prints what and when. Thus 181 marks JSON.awk printing to stdout when (and only when) STREAM=1 is set. While 65 marks the stub function apply in action when (and only when) STREAM=0 is set.

for STREAM in 1 0; do for BRIEF in 0 1; do
  echo BRIEF=$BRIEF STREAM=$STREAM
  echo '[ {"a": 1} ]' | awk -f JSON.awk -v BRIEF=$BRIEF -v STREAM=$STREAM -
done; done
BRIEF=0 STREAM=1
181 [0,"a"]	1
181 [0]	{"a":1}
181 []	[{"a":1}]
BRIEF=1 STREAM=1
181 [0,"a"]	1
BRIEF=0 STREAM=0
65 [0,"a"]	1
65 [0]	{"a":1}
BRIEF=1 STREAM=0

Now, what's with the last result BRIEF=1 STREAM=0 but no output from the stub function apply? Congratulations and thank you for discovering a bug! I consider this a bug of the stub function, not of JSON.awk itself. After all, the stub function is a placeholder for your custom processing. Of course, I still need to fix apply by changing < to <= in the exit condition of the for loop. I will do it soon, and I will better clarify the role of BRIEF, STREAM and apply, both in the documentation and in the source code comments.

@step-
Copy link
Owner

step- commented Feb 26, 2019

All done in commit 4089faf. Please let me know if you have any concerns. Thank you.

@aya
Copy link
Author

aya commented Feb 28, 2019

Thank you for explanations and documentation update. Indeed I misunderstood the purpose of function apply. I was expecting to embed the JSON.awk without editing it, and was calling it with an @include statment as below :

$ declare -A "$(aws deploy get-deployment --deployment-id $DEPLOYMENT_ID \
  |awk -v STREAM=0 -v ARRAY="DEPLOYMENT_GITHUB" -v KEYS='"deploymentInfo","revision","gitHubLocation","(commitId|repository)"' '
    @include "json.awk";
{
    array = sprintf("%s=(",ARRAY);
    regex = "["KEYS"]";
    for (key in JPATHS) {
        if( JPATHS[key] ~ KEYS ) {
            n=patsplit(JPATHS[key], path, "\"([^\"]+)\"");
            array = sprintf("%s [%s]=%s", array, path[n-1], path[n]);
        }
    }
    array = sprintf("%s )", array);
    printf "%s", array;
}' -
)"
$ echo $DEPLOYMENT_GITHUB['repository'];
$ echo $DEPLOYMENT_GITHUB['commitId'];

Embedding JSON.awk with @include or -f statement would allow me to easily upgrade with your futur versions. Maybe you should provide a default behaviour allowing to keep your JSON.awk unspoiled ?

PS. I think you still have a different default STREAM value between the code (STREAM=1) and the FAQ (STREAM=0).
PPS. I close the PR as the code provided is not relevant.

@aya aya closed this Feb 28, 2019
@step-
Copy link
Owner

step- commented Feb 28, 2019

Thank you for you comments. You're right about providing some mechanism to embed JSON.awk as is without needing to modify apply. I will open a new issue for an enhancement request, and you're welcome to comment with your own ideas. The way you're currently embedding JSON.awk is good. The problem is that it's gawk specific. POSIX awk and mawk don't support @include. I would like to find a generic mechanism that works for all awk implementations, if there is one.

@aya
Copy link
Author

aya commented Feb 28, 2019

You're right, @include works only with gawk (and patsplit too).
An awk/mawk compatible solution would be to write your own custom.awk file and call both scripts ?

cat json |awk -f JSON.awk -f custom.awk -

@step-
Copy link
Owner

step- commented Feb 28, 2019

Yes, maybe, in this order -f custom.awk -f JSON.awk.

Just some thoughts now.

A user is required to call awk with -f custom.awk, which must define function apply (even if it does nothing). It's a bit user-unfriendly but awk needs to be able to call apply in its main loop.

@aya
Copy link
Author

aya commented Feb 28, 2019

So, what about adding a test in your apply function ?

function apply (ary, size,   i) { # stub {{{
    if (0 == EMBED)
        for (i=1; i<size; i++)
                print ary[i]
}

It would allow me to pass through the apply function and play with the JPATHS array in my custom script instead.

@step-
Copy link
Owner

step- commented Feb 28, 2019

I think it would need to call apply conditionally. JSON.awk can't redefine apply. Maybe re-writing line 52 like this (untested) would work across all awks.

		if(0 == STREAM) {
			apply(JPATHS, NJPATHS)
		}

Since apply is a stub that does nothing useful for JSON.awk, it only needs to be called back into the application that embeds JSON.awk, which is already signaled by setting STREAM=0. So variable EMBED isn't needed. Does it make sense?

@aya
Copy link
Author

aya commented Mar 1, 2019

Yes, sure ! That's what I'm trying to do in this PR, calling apply only when (1 == STREAM).
I can't redefine apply function outside of JSON.awk, so I cancel the call to this function.
In order to get an apply function working inside JSON.awk too, I think you need an extra EMBED variable to call apply when (0 == EMBED && 0 == STREAM).

@step-
Copy link
Owner

step- commented Mar 1, 2019

what I'm trying to do in this PR, calling apply only when (1 == STREAM)

So you want for JSON.awk to output the JSON paths (STREAM=1) and to fill JPATHS so your redefined apply function can also process JPATHS?
Why can't you have your apply function do both things - print JSON paths and also process JPATHS, which are the same as the JSON paths?

In order to get an apply function working inside JSON.awk too, I think you need an extra EMBED variable to call apply when (0 == EMBED && 0 == STREAM).

I will not define an apply function inside JSON.awk anymore; I don't need one. apply is strictly for embedding. Regular JSON.sh output occurs when STREAM=1 otherwise an externally defined apply will be called. I can't see why I'd need to add an EMBED flag (see my previous point).

@aya
Copy link
Author

aya commented Mar 1, 2019

No, I want JSON.awk to not output the JSON when STREAM=0.
Calling apply function when STREAM=0 actually output the JSON to stdout what's not the expected behavior (IMHO).
Removing apply function lead to the expected behavior (at least for me).

@step-
Copy link
Owner

step- commented Mar 1, 2019

No, I want JSON.awk to not output the JSON when STREAM=0.

That is what will happen.

Removing apply function lead to the expected behavior (at least for me).

With the enhancement that I described there will be no apply function to remove from JSON.awk. Your application will need to define an apply function, which JSON.awk will call if and only if 0 == STREAM.

@aya
Copy link
Author

aya commented Mar 1, 2019

perfect for me :)
But you won't be able to use JSON.awk anymore with STREAM=0 without defining the apply function ?

@step-
Copy link
Owner

step- commented Mar 2, 2019

But you won't be able to use JSON.awk anymore with STREAM=0 without defining the apply function ?

Correct. I see no purpose in calling JSON.awk with STREAM=0 and no apply function because when STREAM=0 JSON.awk per se outputs nothing, and no function is processing jpaths.

Anyway, I have pushed branch issue-12 which implements and documents the new interface. Function apply has become three new callback functions: cb_jpaths, cb_fails and cb_fails1. Possible options are still STREAM and BRIEF as before.

Please take a look and let me know what you think of it. Thank you.

@step-
Copy link
Owner

step- commented Mar 6, 2019

@aya did you see my comment? If you don't have time right now, that's OK, but let me know you've seen it, please.

@aya
Copy link
Author

aya commented Mar 11, 2019

I tested your new branch.
There is a little type in JSON.aws calling cb_fail1(msg) function instead of cb_fails1(msg).
This is a bit more restrictive as I must declare 3 functions instead of processing the JPATHS array directly, but it works as expected !
Thank you.

@step-
Copy link
Owner

step- commented Mar 15, 2019

@aya thank you for testing and for your comments. New commit it b551c16 clarifies cb_fail1. Please note that I settled for "fail1", not "fails1". Alright, I think it's ready for version 1.2. I'll merge it into master in a couple of days. Many thanks for your helpful comments and accurate reviews.

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