Add argument names #165
Add argument names #165
Conversation
yanivagman
force-pushed
the
add_arg_names
branch
3 times, most recently
from
30abcca
to
0183639
Compare
yanivagman
force-pushed
the
add_arg_names
branch
from
0183639
to
aa9b53c
Compare
| @@ -63,10 +63,11 @@ type Event struct { | |||
| EventName string `json:"eventName"` | |||
| ArgsNum int `json:"argsNum"` | |||
| ReturnValue int `json:"returnValue"` | |||
| ArgsNames []string `json:"argsNames"` | |||
There was a problem hiding this comment.
I think we should not just add the argnames array to the Event type here, but instead combine it with the args array, probably as a map[string]interface{}. The reason we created this Event type is that we could decouple the internal data structures (context, []args) that are required to process events from the external data type (Event) that is used to communicate events externally (gob) and for printing (printer). This is a good opportunity to take use the Event struct to adapt the internal representation (2 arrays) which were convenient for processing but not for consuming, and adjust it to be convenient for consuming (one map).
There was a problem hiding this comment.
I'm handling this in some different way in my next PR which is based on this one. We can change this internal representations after these will be merged
| for _, value := range event.Args { | ||
| fmt.Fprintf(p.out, "%v ", value) | ||
| for i, value := range event.Args { | ||
| fmt.Fprintf(p.out, "%s: %v ", event.ArgsNames[i], value) |
There was a problem hiding this comment.
you have only handled the table printer and not the others, but I think the other will be naturally fixed if address my other comment fro above
There was a problem hiding this comment.
Nothing to do in the others - the json printer will just marshal it as well, same for the gob which will encode as well
| argsNames[i] = argName | ||
| } else { | ||
| t.handleError(err) | ||
| continue |
There was a problem hiding this comment.
do we want to fail here or to just skip the tag parsing? (I'm not sure myself)
There was a problem hiding this comment.
This is definetely an error. If an argtag doesn't have an argName, this means something went wrong in the event decoding, as this shouldn't happen
| argsTags[i] = argTag | ||
| args[i] = argVal | ||
| argName, ok := argNames[argTag] | ||
| if ok { | ||
| argsNames[i] = argName |
There was a problem hiding this comment.
The event processing code doesn't care about the arg names, it just needs their tags. the names are for printing only, so I think this code doesn't belongs here rather in the newEvent code
There was a problem hiding this comment.
You are right. I'm handling exactly this in my next PR
No description provided.