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/pin: use new cmds lib #5674

Merged
merged 5 commits into from
Nov 7, 2018
Merged

Conversation

overbool
Copy link
Contributor

Refer: #5664

License: MIT
Signed-off-by: Overbool [email protected]

core/commands/pin.go Outdated Show resolved Hide resolved
core/commands/pin.go Outdated Show resolved Hide resolved
core/commands/pin.go Outdated Show resolved Hide resolved
core/commands/pin.go Outdated Show resolved Hide resolved
core/commands/pin.go Outdated Show resolved Hide resolved
core/commands/pin.go Outdated Show resolved Hide resolved
@Stebalien Stebalien mentioned this pull request Oct 31, 2018
73 tasks
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.

Due to an oddity with how parsing arguments from Stdin works, when ever you have a command that can accept more than one argument from Stdin you must either call req.ParseBodyArgs() before using req.Arguments or iterate over them with req.BodyArgs(), with the former being preferred for now. If you don't only the first argument will be read from Stdin.

You can test this my trying to pass two arguments via the command line to an affected command and then try again via Stdin.

@overbool
Copy link
Contributor Author

overbool commented Nov 2, 2018

@kevina I found that req.Arguments only contain the first arg if I pass two arguments from Stdin

@kevina
Copy link
Contributor

kevina commented Nov 2, 2018

@kevina I found that req.Arguments only contain the first arg if I pass two arguments from Stdin.

Correct. To fix this you need to call req.ParseBodyArgs() before using req.Arguments.

@kevina
Copy link
Contributor

kevina commented Nov 7, 2018

@overbool this needs a rebase now.

I would like to get this in ASAP. Let me know if you need any help with this.

@overbool overbool force-pushed the refactor/commands/pin branch 7 times, most recently from 4c19660 to c7d6589 Compare November 7, 2018 05:55
@overbool
Copy link
Contributor Author

overbool commented Nov 7, 2018

@kevina Sorry for my delay. I had replaced the Encoder with PostRun and fixed some bugs. Could u help me review it again?

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.

One nit. But otherwise LGTM.

core/commands/pin.go Outdated Show resolved Hide resolved
core/commands/pin.go Outdated Show resolved Hide resolved
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.

LGTM.

@Stebalien
Copy link
Member

So, technically, this breaks curl http://localhost:5001/api/v0/pin/add/...?enc=text. I believe the correct way to do this is: overbool#1. That is, handle status in a post-run but forward everything else to the encoder.

@ghost ghost assigned Stebalien Nov 7, 2018
@ghost ghost added the status/in-progress In progress label Nov 7, 2018
overbool and others added 5 commits November 7, 2018 13:19
License: MIT
Signed-off-by: Overbool <[email protected]>
License: MIT
Signed-off-by: Overbool <[email protected]>
License: MIT
Signed-off-by: Overbool <[email protected]>
License: MIT
Signed-off-by: Overbool <[email protected]>
Otherwise, we break the HTTP API (slightly).

License: MIT
Signed-off-by: Steven Allen <[email protected]>
@Stebalien Stebalien merged commit b622e33 into ipfs:master Nov 7, 2018
@ghost ghost removed the status/in-progress In progress label Nov 7, 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