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

minor cleanup for cli.Run #219

Closed
wants to merge 2 commits into from
Closed

minor cleanup for cli.Run #219

wants to merge 2 commits into from

Conversation

djdv
Copy link
Contributor

@djdv djdv commented Sep 20, 2021

I was going over this section and I'm pretty sure all of it is redundant.
(Someone please double check it though)

For 6e8d6f4 d37ab81
The nil checks for req are removed because Parse states "This function never returns nil, even on error."
So these conditions should never change / need to be checked.

For c9f36de 7dc3772
Parse already does this internally (and in a more formal way) to the Command before returning it (attached as req.Command).

// if no encoding was specified by user, default to plaintext encoding
// (if command doesn't support plaintext, use JSON instead)
if enc := req.Options[cmds.EncLong]; enc == "" {
if req.Command.Encoders != nil && req.Command.Encoders[cmds.Text] != nil {
req.SetOption(cmds.EncLong, cmds.Text)
} else {
req.SetOption(cmds.EncLong, cmds.JSON)
}
}
return req, nil
}

Request is supplied by `Parse` which states:
"This function never returns nil, even on error."
making these checks unnecessary.
`Parse` already does this internally, which is where our `req` is
derived from.
@djdv
Copy link
Contributor Author

djdv commented Sep 20, 2021

Tiny force push for consistency.
printMetaHelp directly above printHelp was already referencing .Path directly.
This just matches that.

go-ipfs-cmds/cli/run.go

Lines 50 to 54 in 87b5c50

// this is a message to tell the user how to get the help text
printMetaHelp := func(w io.Writer) {
cmdPath := strings.Join(req.Path, " ")
fmt.Fprintf(w, "Use '%s %s --help' for information about this command\n", cmdline[0], cmdPath)
}

diff --git a/cli/run.go b/cli/run.go
index 567f3ac..0317cca 100644
--- a/cli/run.go
+++ b/cli/run.go
@@ -59,9 +59,7 @@ func Run(ctx context.Context, root *cmds.Command,
 			helpFunc = LongHelp
 		}
 
-		path := req.Path
-
-		if err := helpFunc(cmdline[0], root, path, w); err != nil {
+		if err := helpFunc(cmdline[0], root, req.Path, w); err != nil {
 			// This should not happen
 			panic(err)
 		}

@djdv
Copy link
Contributor Author

djdv commented Sep 20, 2021

For c9f36de 7dc3772
Parse already does this internally (and in a more formal way) to the Command before returning it (attached as req.Command).

Nope. This isn't correct.
While this does happen in Parse, it's only when --enc is not provided.

I'm going to look at this again later.
Either that commit can just be dropped, or a new one can replace it.
I'm thinking that Parse could probably be changed in a way that it handles this, but it might not make sense to.
Need to look at it with non-tired eyes.
But probably something like

if enc := req.Options[cmds.EncLong]; enc == "" || enc == cmds.Text { 

inside Parse()

@djdv
Copy link
Contributor Author

djdv commented Sep 21, 2021

I'm pretty sure the change to Parse would be fine, but without good test coverage none of this is worth the risks.

@djdv djdv closed this Sep 21, 2021
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.

1 participant