refactor(cmds): use new cmds #5659

overbool · 2018-10-26T15:19:05Z

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

overbool · 2018-10-26T19:08:57Z

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

core/commands/active.go

djdv

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

core/commands/config.go

djdv · 2018-10-26T20:56:49Z

core/commands/mount_windows.go

-	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")


Mount isn't compatible with Windows yet

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

Stebalien

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

Stebalien · 2018-10-31T01:51:53Z

core/commands/config.go

-			vf, ok := v.(*ConfigField)
-			if !ok {
-				return nil, e.TypeErr(vf, v)
+		return res.Emit(output)


Same, this needs to use EmitOnce.

@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.

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

core/commands/config.go

core/commands/log.go

core/commands/mount_unix.go

core/commands/sysdiag.go

core/commands/mount_unix.go

Stebalien

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

core/commands/mount_unix.go

core/commands/config.go

Stebalien · 2018-11-05T16:55:34Z

core/commands/config.go

-			vf, ok := v.(*ConfigField)
-			if !ok {
-				return nil, e.TypeErr(vf, v)
+		return res.Emit(output)


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

kevina · 2018-11-06T03:43:40Z

core/commands/mount_unix.go

-		},
+	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)


I would put this in two separate Fprintfs.

License: MIT Signed-off-by: Overbool <overbool.xu@gmail.com>

overbool requested a review from Kubuxu as a code owner October 26, 2018 15:19

overbool force-pushed the refactor/use-new-cmds branch from 1b85ee2 to cd337ac Compare October 26, 2018 15:35

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 force-pushed the refactor/use-new-cmds branch from acff1dd to 1e8fcb1 Compare October 26, 2018 16:02

overbool changed the title ~~refactor(cmds): use new cmds in (diag, config, mount, log)~~ refactor(cmds): use new cmds Oct 26, 2018

overbool force-pushed the refactor/use-new-cmds branch 4 times, most recently from d66114a to 8c954ff Compare October 26, 2018 18:01

Stebalien mentioned this pull request Oct 26, 2018

Commands Library Upgrade #5664

Closed

73 tasks

overbool force-pushed the refactor/use-new-cmds branch from 8c954ff to 8f49a11 Compare October 26, 2018 18:32

Stebalien reviewed Oct 26, 2018

View reviewed changes

core/commands/active.go Outdated Show resolved Hide resolved

djdv reviewed Oct 26, 2018

View reviewed changes

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

Stebalien requested changes Oct 31, 2018

View reviewed changes

overbool force-pushed the refactor/use-new-cmds branch 3 times, most recently from be67020 to ea50cad Compare November 1, 2018 02:15

overbool mentioned this pull request Nov 3, 2018

Remove MessageTextMarshaler once its no longer needed #5722

Closed

Stebalien reviewed Nov 5, 2018

View reviewed changes

core/commands/mount_unix.go Outdated Show resolved Hide resolved

Stebalien requested changes Nov 5, 2018

View reviewed changes

overbool force-pushed the refactor/use-new-cmds branch 2 times, most recently from fea0cda to f7a5151 Compare November 6, 2018 02:26

kevina reviewed Nov 6, 2018

View reviewed changes

overbool force-pushed the refactor/use-new-cmds branch from f7a5151 to 51b05c6 Compare November 6, 2018 06:03

overbool added 2 commits November 6, 2018 13:06

refactor(cmds): use new cmds lib in diag

b2badfb

License: MIT Signed-off-by: Overbool <overbool.xu@gmail.com>

refactor(cmds): use new cmds lib in config

b1efb99

License: MIT Signed-off-by: Overbool <overbool.xu@gmail.com>

overbool added 6 commits November 6, 2018 13:06

refactor(cmds): use new cmds lib in mount

27bc0ec

License: MIT Signed-off-by: Overbool <overbool.xu@gmail.com>

refactor(cmds): use new cmds lib in log

35959d6

License: MIT Signed-off-by: Overbool <overbool.xu@gmail.com>

fix(cmd/diag): inorder output

d26f97a

License: MIT Signed-off-by: Overbool <overbool.xu@gmail.com>

commands: use io.Writer in tabwriter

46902e9

License: MIT Signed-off-by: Overbool <overbool.xu@gmail.com>

commands: use EmitOnce for single object

2c48466

License: MIT Signed-off-by: Overbool <overbool.xu@gmail.com>

cmds: use w.Write directly

0afbafd

License: MIT Signed-off-by: Overbool <overbool.xu@gmail.com>

Stebalien force-pushed the refactor/use-new-cmds branch from 51b05c6 to 0afbafd Compare November 6, 2018 21:06

ghost assigned Stebalien Nov 6, 2018

ghost added the status/in-progress In progress label Nov 6, 2018

Stebalien approved these changes Nov 6, 2018

View reviewed changes

Stebalien merged commit 887be02 into ipfs:master Nov 6, 2018

ghost removed the status/in-progress In progress label Nov 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(cmds): use new cmds #5659

refactor(cmds): use new cmds #5659

overbool commented Oct 26, 2018 •

edited

overbool commented Oct 26, 2018

djdv left a comment

djdv Oct 26, 2018

Stebalien left a comment

Stebalien Oct 31, 2018

overbool Oct 31, 2018

Stebalien Nov 5, 2018

Stebalien left a comment

Stebalien Nov 5, 2018

kevina Nov 6, 2018

refactor(cmds): use new cmds #5659

refactor(cmds): use new cmds #5659

Conversation

overbool commented Oct 26, 2018 • edited

overbool commented Oct 26, 2018

djdv left a comment

Choose a reason for hiding this comment

djdv Oct 26, 2018

Choose a reason for hiding this comment

Stebalien left a comment

Choose a reason for hiding this comment

Stebalien Oct 31, 2018

Choose a reason for hiding this comment

overbool Oct 31, 2018

Choose a reason for hiding this comment

Stebalien Nov 5, 2018

Choose a reason for hiding this comment

Stebalien left a comment

Choose a reason for hiding this comment

Stebalien Nov 5, 2018

Choose a reason for hiding this comment

kevina Nov 6, 2018

Choose a reason for hiding this comment

overbool commented Oct 26, 2018 •

edited