-
Notifications
You must be signed in to change notification settings - Fork 65
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
Node: Add command JSON.RESP #2517
Conversation
ec8a787
to
54ab337
Compare
Signed-off-by: TJ Zhang <tj.zhang@improving.com>
Signed-off-by: TJ Zhang <tj.zhang@improving.com>
Signed-off-by: TJ Zhang <tj.zhang@improving.com>
Signed-off-by: TJ Zhang <tj.zhang@improving.com>
Signed-off-by: TJ Zhang <tj.zhang@improving.com>
af89c5e
to
2695073
Compare
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.
LGTM
Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com>
client: BaseClient, | ||
key: GlideString, | ||
options?: { path: GlideString }, | ||
): Promise<ReturnTypeJson<GlideString>> { |
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.
why is it returning GlideString? the command returns way more than just string, plus we introduced a new return type in #2451
resp is what I call a "universal" commands, can be applied to all types, therefor, ReturnTypeJson
is not suitable for it
Issue link
This Pull Request is linked to issue (URL): #2375
Checklist
Before submitting the PR make sure the following are checked: