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

Added Default logic to refs #2678

Merged
merged 1 commit into from
May 14, 2016
Merged

Conversation

RichardLitt
Copy link
Member

Not sure about the switch statement. There may be a more elegant solution.

Part of #2484

License: MIT
Signed-off-by: Richard Littauer richard.littauer@gmail.com

@@ -50,7 +50,7 @@ NOTE: List all references recursively by using the flag '-r'.
cmds.StringArg("ipfs-path", true, true, "Path to the object(s) to list refs from.").EnableStdin(),
},
Options: []cmds.Option{
cmds.StringOption("format", "Emit edges with given format. Available tokens: <src> <dst> <linkname>."),
cmds.StringOption("format", "Emit edges with given format. Available tokens: <src> <dst> <linkname>.").Default("refs"),
Copy link
Member

Choose a reason for hiding this comment

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

This is wrong.

It should be <dst>.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@RichardLitt RichardLitt force-pushed the feature/add-default-to-refs branch from 3946a96 to 2fb0e23 Compare May 12, 2016 17:45
@Kubuxu
Copy link
Member

Kubuxu commented May 13, 2016

LGTM

@Kubuxu Kubuxu added the RFM label May 13, 2016
@@ -313,6 +313,8 @@ func (rw *RefWriter) WriteEdge(from, to key.Key, linkname string) error {

var s string
switch {
case rw.PrintFmt == "<dst>":
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this case is actually needed, the next case will do what youre doing here already

Copy link
Member

Choose a reason for hiding this comment

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

It might be a simple (possibly premature) optimization.

99% of uses of this command will use this format string.

Copy link
Member Author

Choose a reason for hiding this comment

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

That was my thinking: is this a premature opt?

Copy link
Member

Choose a reason for hiding this comment

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

yeah, i'd call this a premature op. The added time of going through the other case will be dwarfed by the cost of a b58 marshal and api latency

@whyrusleeping whyrusleeping added the need/author-input Needs input from the original author label May 13, 2016
Not sure about the switch statement. There may be a more elegant solution.

Part of #2484

License: MIT
Signed-off-by: Richard Littauer <richard.littauer@gmail.com>
@RichardLitt RichardLitt force-pushed the feature/add-default-to-refs branch from 2fb0e23 to 90421f5 Compare May 14, 2016 00:46
@RichardLitt RichardLitt added topic/docs-ipfs Topic docs-ipfs need/review Needs a review and removed RFM need/author-input Needs input from the original author labels May 14, 2016
@RichardLitt
Copy link
Member Author

Fixed.

@whyrusleeping whyrusleeping merged commit 6cef4ff into master May 14, 2016
@whyrusleeping whyrusleeping deleted the feature/add-default-to-refs branch May 14, 2016 01:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need/review Needs a review topic/docs-ipfs Topic docs-ipfs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants