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

refactor(cmds): use new cmds #5659

Merged
merged 8 commits into from
Nov 6, 2018
Merged

Conversation

overbool
Copy link
Contributor

@overbool overbool commented Oct 26, 2018

Refer: #5664

Use new cmds lib in diag, config, mount, log command.

@overbool overbool requested a review from Kubuxu as a code owner October 26, 2018 15:19
@overbool overbool changed the title refactor(cmds): use new cmds in diag and config refactor(cmds): use new cmds in (diag, config, mount, log) Oct 26, 2018
@overbool overbool changed the title refactor(cmds): use new cmds in (diag, config, mount, log) refactor(cmds): use new cmds Oct 26, 2018
@overbool overbool force-pushed the refactor/use-new-cmds branch 4 times, most recently from d66114a to 8c954ff Compare October 26, 2018 18:01
@Stebalien Stebalien mentioned this pull request Oct 26, 2018
73 tasks
@overbool
Copy link
Contributor Author

@Stebalien CI checks had passed, could u help me review it?

core/commands/active.go Outdated Show resolved Hide resolved
Copy link
Contributor

@djdv djdv left a comment

Choose a reason for hiding this comment

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

Generally looks good to me, but I have not followed the cmdslib growth that closely.
There's 2 thing that stood out to me, but I'm looking to @keks and/or @Stebalien for guidance here.
Everything else seems in line with the typical cmds upgrade pattern.

core/commands/active.go Outdated Show resolved Hide resolved
core/commands/config.go Outdated Show resolved Hide resolved
Run: func(req cmds.Request, res cmds.Response) {
res.SetError(errors.New("Mount isn't compatible with Windows yet"), cmdkit.ErrNormal)
Run: func(req *cmds.Request, res cmds.ResponseEmitter, env cmds.Environment) error {
return errors.New("Mount isn't compatible with Windows yet")
Copy link
Contributor

Choose a reason for hiding this comment

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

Mount isn't compatible with Windows yet

I'll fix this in a separate PR ;^)

@overbool overbool force-pushed the refactor/use-new-cmds branch 3 times, most recently from 2291c3e to 47c87a6 Compare October 27, 2018 06:10
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

So, it turns out go-ipfs has no issues with Emit versus EmitOnce because, internally, we always assume responses are streams of objects. However, js-ipfs changes their API based on whether we use EmitOnce or Emit so we need to make sure to use EmitOnce where appropriate.

Historically, SetOutput was equivalent to EmitOnce when passed an object and Emit when passed a channel. Now that we no longer have a SetOutput function, we need to be careful.

(we should also improve the commands lib API so we don't have to deal with this issue but that's a separate issue).

core/commands/active.go Outdated Show resolved Hide resolved
vf, ok := v.(*ConfigField)
if !ok {
return nil, e.TypeErr(vf, v)
return res.Emit(output)
Copy link
Member

Choose a reason for hiding this comment

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

Same, this needs to use EmitOnce.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Stebalien I found a strange question, when I use cmds.EmitOnce(output) here, it will fail to pass t0063-external.sh testcase. After analyzing the t0063-external.sh. I found that ipfs config key value won't remove repo.lock file successfully. However, ipfs config key value won't print any error or panic.

Copy link
Member

Choose a reason for hiding this comment

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

Hm. That's not good. @magik6k or @kevina, want to look into this?

core/commands/config.go Outdated Show resolved Hide resolved
core/commands/config.go Outdated Show resolved Hide resolved
core/commands/log.go Outdated Show resolved Hide resolved
core/commands/log.go Outdated Show resolved Hide resolved
core/commands/log.go Show resolved Hide resolved
core/commands/mount_unix.go Outdated Show resolved Hide resolved
core/commands/sysdiag.go Outdated Show resolved Hide resolved
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

will also need a rebase and will need to use your new cmdenv.GetConfigRoot.

core/commands/mount_unix.go Outdated Show resolved Hide resolved
core/commands/config.go Outdated Show resolved Hide resolved
core/commands/config.go Outdated Show resolved Hide resolved
core/commands/config.go Outdated Show resolved Hide resolved
vf, ok := v.(*ConfigField)
if !ok {
return nil, e.TypeErr(vf, v)
return res.Emit(output)
Copy link
Member

Choose a reason for hiding this comment

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

Hm. That's not good. @magik6k or @kevina, want to look into this?

@overbool overbool force-pushed the refactor/use-new-cmds branch 2 times, most recently from fea0cda to f7a5151 Compare November 6, 2018 02:26
},
Encoders: cmds.EncoderMap{
cmds.Text: cmds.MakeTypedEncoder(func(req *cmds.Request, w io.Writer, mounts *config.Mounts) error {
fmt.Fprintf(w, "IPFS mounted at: %s\nIPNS mounted at: %s\n", mounts.IPFS, mounts.IPNS)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would put this in two separate Fprintfs.

License: MIT
Signed-off-by: Overbool <overbool.xu@gmail.com>
License: MIT
Signed-off-by: Overbool <overbool.xu@gmail.com>
License: MIT
Signed-off-by: Overbool <overbool.xu@gmail.com>
License: MIT
Signed-off-by: Overbool <overbool.xu@gmail.com>
License: MIT
Signed-off-by: Overbool <overbool.xu@gmail.com>
License: MIT
Signed-off-by: Overbool <overbool.xu@gmail.com>
License: MIT
Signed-off-by: Overbool <overbool.xu@gmail.com>
License: MIT
Signed-off-by: Overbool <overbool.xu@gmail.com>
@ghost ghost assigned Stebalien Nov 6, 2018
@ghost ghost added the status/in-progress In progress label Nov 6, 2018
@Stebalien Stebalien merged commit 887be02 into ipfs:master Nov 6, 2018
@ghost ghost removed the status/in-progress In progress label Nov 6, 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.

5 participants