-
Couldn't load subscription status.
- Fork 0
W-16451183 Wr/file flag #14
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
Conversation
| file: Flags.file({ | ||
| summary: messages.getMessage('flags.file.summary'), | ||
| helpValue: 'file', | ||
| char: 'f', |
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.
you can pass exists to have oclif validate the file:
https://github.com/oclif/core/blob/f38f8270bafa04cdcb8e6d193b808e099e205864/src/flags.ts#L121
also, we haven't been consistent with this but since this command already has --stream-to-file, what do you think about using --request-file instead?
https://github.com/search?q=org%3Asalesforcecli+%2FFlags%5C.file%5C%28%2F&type=code
https://github.com/salesforcecli/cli/wiki/Design-Guidelines-Flags
src/commands/api/request/rest.ts
Outdated
| const fileOptions = flags.file | ||
| ? (JSON.parse(fs.readFileSync(flags.file, 'utf8')) as { | ||
| body?: string; | ||
| header?: string[]; |
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 should be headers (flag is singular because we expect user to pass it multiple times)
src/commands/api/request/rest.ts
Outdated
| : // otherwise it's a stdin, and we use it directly | ||
| flags.body; | ||
| } else { | ||
| body = JSON.stringify(fileOptions.body); |
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.
cc @VivekMChawla @WillieRuemmele
Today --body can be a body string, - to read from stdin or a file path to read the body from.
Since this PR adds support for passing a request as a file I feel we should drop --body from supporting file paths (it's the same feature but we are opening 2 ways to use it).
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.
@WillieRuemmele - I think that @cristiand391's idea to drop support for file paths in the --body flag makes sense.
messages/rest.md
Outdated
|
|
||
| # flags.file.summary | ||
|
|
||
| A json file to store values for header/body/method/url - this is the same format as a Postman Collection Format |
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.
@WillieRuemmele @VivekMChawla I couldn't find a way to export a postman request in the collection format.
If it's not possible then we might not need to follow their format?
context:
by not following their format we leave this open to us adding support for parameterized params later:
https://salesforce.quip.com/5t19A9rykzI3#SdNADAvRyDL
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.
looks good, left a few questions regarding functionality/formats (will ping Vivek) and get back.
|
QA notes: arg/flags override value in body file✅ endpoint as ARG others✅ HTTP method still defaults to body format✅ NOTE: it's missing the {
"url": "limits",
"header": [{
"value": "application/xml"
}]
}
✅ raw body mode, can define request body as JSON (gets stringified in payload) NOTE: it's missing the {
"url": "sobjects/Account/0018G00000bTTg4QAG",
"method": "PATCH",
"body" : {
"raw": {
"name": "Mr Doe"
}
}
}we should either default to |
|
QA update:
✅ fixed:
✅ fixed |
What does this PR do?
adds
--fileflag torequest restcommandmatches postman collection format e.g. keys are body/header/url/method
What issues does this PR fix or reference?
@W-16451183@