-
Notifications
You must be signed in to change notification settings - Fork 35
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 Encoding & File to YAML template #338
base: dev
Are you sure you want to change the base?
Conversation
Thanks for the contribution, is there any chance to update the summary of the PR to get more context on this please? |
I updated the top comment, is that what you need? |
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.
Please update the summary of the diff with examples of this new feature. Can you also please update the Changelog with this new feature
case "json": | ||
opts.ROpts.Encoding = encoding.JSON | ||
case "protobuf": | ||
opts.ROpts.Encoding = encoding.Protobuf |
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.
When using the option -e, --encoding, encoding.Protobuf will match with proto
and not protobuf
.
return err | ||
} | ||
} | ||
|
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.
We should prevent users of using file
and request
together. Right now, File have priority over request, only one should be available
case "raw": | ||
opts.ROpts.Encoding = encoding.Raw | ||
} | ||
|
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.
Line 169
, there is the following line of code overrideParam(&opts.ROpts.RequestJSON, string(body))
. How would it work when the body given in file has another format than JSON? Right now, body could be with format proto, thrift or raw.
opts := newOptions() | ||
err := readYAMLFile("testdata/valid.yab", nil, opts) | ||
assert.NoError(t, err) | ||
} |
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.
Can we add more test please? We should provide the following unit tests:
- Yab template with file + JSON format
- Yab template with file + YAML format
- Yab template with file + RAW format
- Yab template with file + Proto format
- Yab template with file + Thrift format
- Negative test with bad combinaison of options (like File and request together)
Many of the options to yab can be specified in a YAML template. In my case I want to specify a file to read the request data from and also to specify how the request is encoded.
This diff enables these options within the template file.