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

fix ipfs help bug #5557 #5573

Merged
merged 2 commits into from
Nov 6, 2018
Merged

fix ipfs help bug #5557 #5573

merged 2 commits into from
Nov 6, 2018

Conversation

kjzz
Copy link
Contributor

@kjzz kjzz commented Oct 9, 2018

fix issue #5557

License: MIT
Signed-off-by: Kejie Zhang [email protected]

cmd/ipfs/main.go Outdated Show resolved Hide resolved
@djdv
Copy link
Contributor

djdv commented Oct 9, 2018

My review is squashed.
Just a note for the next person.
I'm wondering if we should use the short help description or the long one. -h vs --help.
No strong opinion either way.

@kjzz kjzz closed this Oct 9, 2018
@kjzz kjzz reopened this Oct 9, 2018
cmd/ipfs/main.go Outdated
if os.Args[1] == "help" {
if len(os.Args) > 2 {
os.Args = append(os.Args[:1], os.Args[2:]...)
os.Args = append(os.Args, "-h")
Copy link
Member

Choose a reason for hiding this comment

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

I would switch it to --help as -h is a short help and --help is a long help. (help help help :smile)

Copy link
Member

Choose a reason for hiding this comment

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

Same in line 98.

@kjzz
Copy link
Contributor Author

kjzz commented Oct 10, 2018

@djdv @Kubuxu @Stebalien i have update the pr . And I have changed -h to --help ,please help me review again.Thx.And maybe something wrong with jenkins ci . If you have any time,please help me restart it .

@kjzz kjzz changed the title fix ipfs help bug fix ipfs help bug #5557 Oct 11, 2018
@Stebalien Stebalien requested a review from djdv October 18, 2018 18:27
@kjzz
Copy link
Contributor Author

kjzz commented Oct 29, 2018

@djdv @magik6k If you have any time , can you please help me review my pr ?Thx a lot.

Copy link
Member

@Kubuxu Kubuxu left a comment

Choose a reason for hiding this comment

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

The code seems good to me. One thing about tests. I wouldn't be adding help test cases in various files, instead, I would add a second test case to t0010-basic-commands.sh (similar to All commands accept --help test) that tests `ipfs help .

@djdv
Copy link
Contributor

djdv commented Oct 29, 2018

Sorry for the delay @kjzz
I wasn't very clear.
My initial review was only blocking on the question of using -h or --help.
With the decision to use --help I approve the changes.
But was waiting for input from someone with merge capabilities. (like @Kubuxu, who has posted above)

License: MIT
Signed-off-by: Kejie Zhang <[email protected]>
License: MIT
Signed-off-by: Kejie Zhang <[email protected]>
@kjzz
Copy link
Contributor Author

kjzz commented Oct 30, 2018

Thx for advice @Kubuxu.I have update my pr.Please help me review it again.

@Stebalien
Copy link
Member

This is a bit funky but, well, it was already kind of a hack. This should at least be less surprising for users.

@Stebalien Stebalien merged commit 5716926 into ipfs:master Nov 6, 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.

4 participants