-
Notifications
You must be signed in to change notification settings - Fork 16
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 --json option to get balance as JSON #103
Conversation
Actually, for true JSON support for such commands, we could have a success variable. Only when success:true would the user then read the rest of the values. An alternative would be just to return relevant parameters of ZCN being 0. For now, I will just class invalid JSON as 0. |
@@ -14,6 +15,8 @@ var getbalancecmd = &cobra.Command{ | |||
Long: `Get balance from sharders`, | |||
Args: cobra.MinimumNArgs(0), | |||
Run: func(cmd *cobra.Command, args []string) { | |||
doJSON, _ := cmd.Flags().GetBool("json") |
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.
Check error in case use invalid value is used
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.
--json is bool so requires no parameter. But feel free to change however is best way
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.
@peterlimg do you agree? there is no parameter to check?
@peterlimg what is your opinion on having success field? |
I would suggest to not add the success field, we can return json value when success, and return error directly. We can put the command in |
Okay, sure. |
@peterlimg sorry, I'm not too hot with GitHub, am I required to implement your changes before you can approve and close? |
@peterlimg can this be merged now? |
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
usd - Current USD value, zcn - ZCN amount (float) fmt - Formatted zcn value (mZCN etc.)
ca0e294
to
c6b95b3
Compare
why wasn't this merged? now its conflicting again.. |
usd - Current USD value,
zcn - ZCN amount (float)
fmt - Formatted zcn value (mZCN etc.)
A brief description of the changes in this PR:
Tasks to complete before merging PR:
Associated PRs (Link as appropriate):