Skip to content
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.

Sign up for GitHub

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

Jump to bottom

refactor(rpc): rename channelbalance to balance #1180

Merged
merged 1 commit into from
Sep 10, 2019
Merged

refactor(rpc): rename channelbalance to balance #1180

merged 1 commit into from
Sep 10, 2019

Conversation

krrprr
Copy link
Collaborator

@krrprr krrprr commented Aug 25, 2019

First step for solving #1062

@kilrau kilrau requested review from a user and sangaman August 25, 2019 16:24
krrprr
krrprr commented Aug 25, 2019
View reviewed changes
docs/api.md
@@ -145,103 +145,103 @@



<a name="xudrpc.BanRequest"></a>
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any ideas why BanRequest and BanResponse got involved?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably something to do with alphabetic reordering of the rpc calls and messages, not a big deal.

sangaman
sangaman reviewed Aug 27, 2019
View reviewed changes
Copy link
Collaborator

@sangaman sangaman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments below. Also for the commit message I think refactor makes more sense than feat - and since this affects the rpc layer as well as the cli layer I think it'd be more apt to specify (rpc) than (cli).

proto/xudrpc.proto Outdated
/* Gets the total balance available across all payment channels for one or all currencies. */
rpc ChannelBalance(ChannelBalanceRequest) returns (ChannelBalanceResponse) {
/* Gets the total balance available across all payment channels and wallets for one or all currencies. */
rpc Balance(BalanceRequest) returns (BalanceResponse) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Balance" sounds a little bit ambiguous to me for an API call at least - it almost sounds more like a verb than a noun. How about GetBalance? I'd be ok with balance still for the xucli call for brevity's sake. Thoughts @kilrau?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! getBalance for functions is much better than balance.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GetBalance 👍

docs/api.md
@@ -145,103 +145,103 @@



<a name="xudrpc.BanRequest"></a>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably something to do with alphabetic reordering of the rpc calls and messages, not a big deal.

lib/proto/xudrpc.swagger.json Outdated
"operationId": "AddCurrency",
"responses": {
"200": {
"description": "",
"description": "A successful response.",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you try following the commands in #1186 and then try regenerating the proto files to see if these changes go away?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks much better now. Thank you!

@krrprr krrprr force-pushed the feat/balance-command-renaming branch from d69cff1 to 2c29f40 Compare August 28, 2019 18:12
@krrprr krrprr changed the title feat(cli): rename channelbalance to balance refactor(rpc): rename channelbalance to balance Aug 31, 2019
@krrprr krrprr mentioned this pull request Aug 31, 2019
Merged
@krrprr krrprr requested a review from sangaman September 3, 2019 16:54
ghost
ghost approved these changes Sep 8, 2019
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

sangaman
sangaman requested changes Sep 9, 2019
View reviewed changes
Copy link
Collaborator

@sangaman sangaman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few minor naming changes, then good to merge. Thanks!

proto/xudrpc.proto Outdated Show resolved Hide resolved
proto/xudrpc.proto Outdated Show resolved Hide resolved
proto/xudrpc.proto Outdated Show resolved Hide resolved
@krrprr krrprr force-pushed the feat/balance-command-renaming branch from 2c29f40 to 5c87184 Compare September 9, 2019 16:09
@krrprr krrprr force-pushed the feat/balance-command-renaming branch from 5c87184 to 48e8f94 Compare September 9, 2019 16:11
@krrprr
Copy link
Collaborator Author

krrprr commented Sep 9, 2019

Thanks for the feedback @sangaman! Made the requested changes.

sangaman
sangaman approved these changes Sep 10, 2019
View reviewed changes
Copy link
Collaborator

@sangaman sangaman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@sangaman sangaman merged commit 90c129f into ExchangeUnion:master Sep 10, 2019
@sangaman
Copy link
Collaborator

@krrprr Will you be following up on modifying the way balances are calculated to reflect total balance as well? I figure we'll want the code to line up with the call naming soon.

@kilrau
Copy link
Contributor

kilrau commented Sep 10, 2019 •
edited
Loading

And.. did we ignore GetBalance?

I mean honestly, for me either way is fine just that we all agreed on GetBalance and now it was merged as balance.

Or was that just on the gRPC layer? If so, that was a misunderstanding: I don't think it's a good idea to have cli and grpc named differently unless there is a really good reason to do so.

@sangaman
Copy link
Collaborator

And.. did we ignore GetBalance?

I mean honestly, for me either way is fine just that we all agreed on GetBalance and now it was merged as balance.

I said I'd be fine with balance for the cli command because it's shorter, GetBalance is what it's called on the API. But I'd also be fine with getbalance on the cli, just doesn't make much difference to me.

@kilrau
Copy link
Contributor

kilrau commented Sep 10, 2019

But I'd also be fine with getbalance on the cli, just doesn't make much difference to me.

Right, not much difference - but they should be the same to avoid confusion. So I'd say let's change it to getbalance on the CLI too @krrprr

Sorry for the back and forth!

@krrprr
Copy link
Collaborator Author

krrprr commented Sep 10, 2019

So I'd say let's change it to getbalance on the CLI too

Agreed. I'll do so.

Will you be following up on modifying the way balances are calculated to reflect total balance as well?

Wallet Balance described in #1062 is implemented in #1197. I'll rebase after the balance -> getbalance gets done and merged. Is there a need for Total balance (channel + wallet) or any other value as well?

@kilrau
Copy link
Contributor

kilrau commented Sep 10, 2019

Is there a need for Total balance (channel + wallet) or any other value as well?
See #1062 (comment)

@krrprr
Copy link
Collaborator Author

krrprr commented Sep 10, 2019

Right, I didn't notice the updates. Thanks! I'll delve into it probably tomorrow.

@krrprr krrprr mentioned this pull request Sep 10, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kilrau kilrau kilrau left review comments

@sangaman sangaman sangaman approved these changes

Assignees

@krrprr krrprr

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@krrprr @sangaman @kilrau