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

refact(cmd/repo): repo's sub cmds uses new cmd lib #5677

Merged
merged 3 commits into from
Nov 5, 2018
Merged

refact(cmd/repo): repo's sub cmds uses new cmd lib #5677

merged 3 commits into from
Nov 5, 2018

Conversation

qiwaa
Copy link
Contributor

@qiwaa qiwaa commented Oct 27, 2018

Refer: #5664

License: MIT
Signed-off-by: chenminjian 727180553@qq.com

@qiwaa qiwaa requested a review from Kubuxu as a code owner October 27, 2018 05:47
@qiwaa
Copy link
Contributor Author

qiwaa commented Oct 28, 2018

I have already done this. It looks like CI goes wrong.

Stebalien
Stebalien requested changes Oct 28, 2018
View reviewed changes
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

(our CI is a bit overloaded)

core/commands/repo.go Outdated Show resolved Hide resolved
core/commands/repo.go Outdated Show resolved Hide resolved
core/commands/repo.go Outdated Show resolved Hide resolved
@qiwaa
Copy link
Contributor Author

qiwaa commented Oct 29, 2018

I use repoGcCmd as a reference, so repoGcCmd is also wrong... #5687

@qiwaa
Copy link
Contributor Author

qiwaa commented Oct 30, 2018

Hey, @Stebalien . I have made the appropriate changes to the wrong code. Can you help me review my code again?

@qiwaa qiwaa changed the title refact(cmd/repo): repo fsck and repo version uses new cmd lib refact(cmd/repo): repo's sub cmds uses new cmd lib Oct 30, 2018
Stebalien
Stebalien approved these changes Oct 31, 2018
View reviewed changes
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

LGTM but blocked on go-ipfs release.

@Stebalien Stebalien mentioned this pull request Oct 31, 2018
Closed
73 tasks
kevina
kevina suggested changes Nov 2, 2018
View reviewed changes
Copy link
Contributor

@kevina kevina left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Several minor things, but one potential API change that I want @Stebalien okay on.

Otherwise LGTM.

core/commands/repo.go Outdated Show resolved Hide resolved
core/commands/repo.go Outdated Show resolved Hide resolved
core/commands/repo.go Outdated Show resolved Hide resolved
core/commands/repo.go Outdated Show resolved Hide resolved
@qiwaa qiwaa changed the title refact(cmd/repo): repo's sub cmds uses new cmd lib [WIP]refact(cmd/repo): repo's sub cmds uses new cmd lib Nov 3, 2018
@qiwaa
refact(cmd/repo): repo fsck and repo version uses new cmd lib
f7b6ee3 
License: MIT
Signed-off-by: chenminjian <727180553@qq.com>
@qiwaa
refact(cmd/repo): repo verify uses new cmd lib
5afa7eb 
License: MIT
Signed-off-by: chenminjian <727180553@qq.com>
@qiwaa
refact(cmd/repo): normalize fsck cmd name
ff27f9f 
License: MIT
Signed-off-by: chenminjian <727180553@qq.com>
@qiwaa qiwaa changed the title [WIP]refact(cmd/repo): repo's sub cmds uses new cmd lib refact(cmd/repo): repo's sub cmds uses new cmd lib Nov 3, 2018
kevina
kevina approved these changes Nov 3, 2018
View reviewed changes
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

@Stebalien Stebalien merged commit 254dda2 into ipfs:master Nov 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Stebalien Stebalien Stebalien approved these changes

@kevina kevina kevina approved these changes

@Kubuxu Kubuxu Awaiting requested review from Kubuxu

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@qiwaa @Stebalien @kevina