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

commands/object: use new cmds #5666

Merged
merged 4 commits into from
Nov 2, 2018

Conversation

overbool
Copy link
Contributor

Refer: #5664
License: MIT
Signed-off-by: Overbool [email protected]

@overbool overbool requested a review from Kubuxu as a code owner October 26, 2018 19:27
@overbool overbool changed the title [WIP] commands/object: use new cmds commands/object: use new cmds Oct 27, 2018
@overbool
Copy link
Contributor Author

@Stebalien CI checks have passed, could u help me review it?

@Stebalien Stebalien mentioned this pull request Oct 31, 2018
73 tasks
Copy link
Contributor

@kevina kevina left a comment

Choose a reason for hiding this comment

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

LGTM

cmdkit "gx/ipfs/Qmde5VP1qUkyQXKCfmEUA7bP64V2HAptbJ7phuPp7jXWwg/go-ipfs-cmdkit"
)

const (
verboseOptionName = "verbose"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason for this?

Nothing wrong with, but it's not very consistent as we don't use contacts for any of the other options in this file.

Personally, I would err on the side of not using the constant, but others may disagree.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kevina version is used in two places, so I use the constant.

Copy link
Contributor

Choose a reason for hiding this comment

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

So is almost every other option, or am I missing something.

In any case it not that big of a deal.

Copy link
Member

Choose a reason for hiding this comment

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

Most of the commands have actually been changed to use constants for these.

}

res.SetOutput(data)
return res.Emit(data)
Copy link
Member

Choose a reason for hiding this comment

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

Needs to use EmitOnce.

@Stebalien Stebalien dismissed their stale review November 2, 2018 18:07

(nevermind, lgtm)

@ghost ghost assigned Stebalien Nov 2, 2018
@ghost ghost added the status/in-progress In progress label Nov 2, 2018
@Stebalien Stebalien merged commit 6926812 into ipfs:master Nov 2, 2018
@ghost ghost removed the status/in-progress In progress label Nov 2, 2018
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.

3 participants