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: fix refs 'edges' option work #3007

Merged
merged 1 commit into from
Aug 1, 2016

Conversation

Kubuxu
Copy link
Member

@Kubuxu Kubuxu commented Jul 28, 2016

also change it to use format instead of separate variable

Resolves #3001

License: MIT
Signed-off-by: Jakub Sztandera [email protected]

also change it to use format instead of separate variable

License: MIT
Signed-off-by: Jakub Sztandera <[email protected]>
@Kubuxu Kubuxu added this to the ipfs-0.4.3-rc2 milestone Jul 28, 2016
@Kubuxu Kubuxu added the need/review Needs a review label Jul 28, 2016
if err != nil {
res.SetError(err, cmds.ErrNormal)
return
}
if edges {
if format != "<dst>" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't format be an empty string (rather than "<dst>") if it isn't provided?

Copy link
Member

Choose a reason for hiding this comment

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

no because: .Default("<dst>")

@JesseWeinstein
Copy link
Contributor

JesseWeinstein commented Jul 29, 2016

This has no tests for --format. While it looks like it should work, it would be much better if that was actually tested. I'll see about making a later PR to add them, unless you feel like doing so in this PR.

(edit: Since I've now confirmed that --format works in 0.4.2, there's no need to add tests for it in this PR. I'll still work on making a separate one, though.)

@JesseWeinstein
Copy link
Contributor

Interesting -- in 0.4.2, --edges does work, if you also explicitly set --format to an empty string, like so: ipfs refs --format '' -e QmYwAPJzv5CZsnA625s3Xf2nemtYgPpHdWEz79ojWnPbdG

@@ -315,8 +322,6 @@ func (rw *RefWriter) WriteEdge(from, to key.Key, linkname string) error {
s = strings.Replace(s, "<src>", from.B58String(), -1)
s = strings.Replace(s, "<dst>", to.B58String(), -1)
s = strings.Replace(s, "<linkname>", linkname, -1)
case rw.PrintEdge:
s = from.B58String() + " -> " + to.B58String()
default:
s += to.B58String()
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need this default case? It would only trigger if --format is explicitly set to the empty string, which seems like a corner case better handled by either an error, or actually printing an empty string for each edge, rather than overriding the explicit intention of the user.

@whyrusleeping whyrusleeping merged commit 149819f into master Aug 1, 2016
@whyrusleeping whyrusleeping deleted the feature/refs-format-regression branch August 1, 2016 11:45
@whyrusleeping
Copy link
Member

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants