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

Revert "Merge pull request #2657 from ipfs/feature/add-defaults-to-add" #3239

Closed
wants to merge 1 commit into from
Closed

Revert "Merge pull request #2657 from ipfs/feature/add-defaults-to-add" #3239

wants to merge 1 commit into from

Conversation

RichardLitt
Copy link
Member

I think I did this incorrectly, but I am not sure. Why should a revert have merge conflicts? This is an attempt at fixing #3164.

@RichardLitt RichardLitt added status/in-progress In progress need/review Needs a review labels Sep 19, 2016
@RichardLitt
Revert "Merge pull request #2657 from ipfs/feature/add-defaults-to-add"
5c7a393 
This reverts commit da4a4ac, reversing
changes made to 518f7e0.

License: MIT
Signed-off-by: Richard Littauer <richard.littauer@gmail.com>
@RichardLitt RichardLitt mentioned this pull request Sep 19, 2016
Closed
@Kubuxu Kubuxu self-assigned this Nov 2, 2016
@whyrusleeping whyrusleeping added status/ready Ready to be worked and removed status/in-progress In progress labels Nov 28, 2016
kevina
kevina suggested changes Dec 1, 2016
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.

Except for the one change I noted. I think this is okay. In order for me to really say it is okay I would have needed to do the revert itself. There is some stuff with the progress bar setting I don't fully understand.

core/commands/add.go
cmds.StringOption(chunkerOptionName, "s", "Chunking algorithm to use."),
cmds.BoolOption(pinOptionName, "Pin this object when adding.").Default(true),
cmds.BoolOption(pinOptionName, "Pin this object when adding. Default: true."),
Copy link
Contributor

Choose a reason for hiding this comment

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

This should still be ".Default(true)."

core/commands/add.go

if !pin_found { // default
dopin = true
}

Copy link
Contributor

Choose a reason for hiding this comment

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

This if !pin_found {...} should not be necessary. See earlier comment.

core/commands/add.go
}

req.SetOption(progressOptionName, progress)

sizeFile, ok := req.Files().(files.SizeFile)
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: I don't understand what is going on here. Why are we setting progressOptionName if it is not found. See my note in the PostRun function.

core/commands/add.go
@@ -242,7 +247,7 @@ You can now refer to the added file in a gateway, like so:
return
}

progress, _, err := req.Option(progressOptionName).Bool()
progress, prgFound, err := req.Option(progressOptionName).Bool()
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: Okay so how is it that prgFound can be false, it we explicitly set it the PreRun. I am very unsure of the correct behavior here. What was the original intention. @whyrusleeping @Kubuxu any ideas?

@kevina
Copy link
Contributor

kevina commented Dec 1, 2016

Everyone. I will be willing to just take over this pull request and redo it if it would be easier, but I don't want to step on anyone's toes.

@RichardLitt
Copy link
Member Author

My toes are very small. It is hard to step on them. :)

Take it away! Thank you, @kevina.

@RichardLitt
Copy link
Member Author

Regarding your questions: I don't know, I am not invested, and I won't pretend my edits were high quality. They were the best I managed at the time, but realistically, if you don't understand something I did, that's my fault, not yours. Feel free to close this PR unmerged if that is the easiest way forward.

@kevina
Copy link
Contributor

kevina commented Dec 1, 2016

@RichardLitt, Okay, I will just redo this in the next day or so.

@kevina kevina self-assigned this Dec 1, 2016
@kevina
Copy link
Contributor

kevina commented Dec 4, 2016

@RichardLitt okay I redid the revert in #3464.

Note that your revert was fine, when I did it I got the same results. I just chose to resolve a conflict slightly differently.

@kevina kevina closed this Dec 4, 2016
@kevina kevina removed the status/ready Ready to be worked label Dec 4, 2016
@RichardLitt
Copy link
Member Author

Thank you, @kevina!

@RichardLitt RichardLitt deleted the feat/revert-da4a4ac branch December 5, 2016 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevina kevina kevina requested changes

Assignees

@kevina kevina

@Kubuxu Kubuxu

Labels
need/review Needs a review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@RichardLitt @kevina @whyrusleeping @Kubuxu