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.

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

multi: move extension commands into associated normal command files. #1238

Merged
merged 1 commit into from
Jun 3, 2018

Conversation

dnldd
Copy link
Member

@dnldd dnldd commented May 29, 2018

This consolidates all commands and tests into their respective normal
files. BtcdConnectedNtfn has been renamed DcrdConnectedNtfn.

Resolves #1220

@davecgh davecgh added this to the 1.3.0 milestone Jun 1, 2018
Copy link
Member

@davecgh davecgh left a comment

Choose a reason for hiding this comment

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

Round 1 of review. I'll review some more later.

@@ -28,6 +32,16 @@ const (
// ANOneTry indicates the specified host should try to connect once,
// but it should not be made persistent.
ANOneTry AddNodeSubCmd = "onetry"

// NConnect indicates the specified host that should be connected to.
NConnect NodeSubCmd = "connect"
Copy link
Member

Choose a reason for hiding this comment

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

Please do these in separate blocks next to the type it's defined for so it's clear it is an enum.

type AddNodeSubCmd string

const(
  ANAdd AddNodeSubCmd = "add"
  ...
)

type NodeSubCmd string

const (
  NConnect NodeSubCmd = "connect"
  ...
)

Also, please define it next to the command it applies to.

@@ -1017,6 +1017,152 @@ func TestChainSvrCmds(t *testing.T) {
Message: "test",
},
},
{
name: "debuglevel",
Copy link
Member

Choose a reason for hiding this comment

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

Please alphabetize all of these for consistency.

},
},
{
name: "debuglevel",
Copy link
Member

Choose a reason for hiding this comment

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

Already defined above.

Copy link
Member

@davecgh davecgh left a comment

Choose a reason for hiding this comment

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

Nice work overall.

I noticd that the SendToMultiSigResult type was removed, but doesn't appear to have been added back to walletsvrresults.go as it should.

Also, more comments inline.

@@ -1139,6 +1139,121 @@ func TestWalletSvrCmds(t *testing.T) {
NewPassphrase: "new",
},
},
{
name: "sweepaccount - optionals provided",
Copy link
Member

Choose a reason for hiding this comment

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

Please alphabetize all of these for consistency.


// CreateMultiSigResult models the data returned from the createmultisig
// command.
type CreateMultiSigResult struct {
Copy link
Member

Choose a reason for hiding this comment

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

There are a lot of results that aren't alphabetized here. The only time they shouldn't be is if they are subresults of a field of a specific command in which case that subresult should come before the command result that uses it.

@dnldd dnldd force-pushed the combine_ext_cmds branch 3 times, most recently from ec6621e to 7f0c77b Compare June 2, 2018 23:47
This consolidates all commands and tests into their respective normalfiles. BtcdConnectedNtfn has been renamed DcrdConnectedNtfn.
@davecgh davecgh merged commit 4840587 into decred:master Jun 3, 2018
@dnldd dnldd deleted the combine_ext_cmds branch June 4, 2018 23:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants