CLI and API command output encoding #215

jbouwman · 2021-08-19T17:32:38Z

Following the approach described in #115, define a new method signature on Command that supports full processing of the Response object when text encoding is requested.

Add an encoding check and dispatch to DisplayCLI in local, http client, and http handler code paths.

Unblocks resolution of encoding option processing in multiple go-ipfs issues.

ipfs --encoding=json ls not returning JSON kubo#7050 json encoding for ls
Add command does not support json encoding kubo#1121 json encoding for add
ipfs stats bw doesn't support --enc json kubo#5594 json encoding for stats bw

Closes #115

Stebalien

Thanks for implementing this, it's been bugging me for a while... And thanks for making it possible to incrementally switch over, that'll make this much easier to land.

executor.go

http/responseemitter.go

http/client.go

executor.go

Stebalien · 2021-09-07T15:45:15Z

Sorry for the very slow response, things that touch this library tend to be scary so I tend to procrastinate. This looks quite reasonable, so I'll be sure to make future review cycles shorter.

jbouwman · 2021-09-07T17:29:44Z

Sorry for the very slow response, things that touch this library tend to be scary so I tend to procrastinate. This looks quite reasonable, so I'll be sure to make future review cycles shorter.

No worries whatsoever. Thanks very much for the review.

Following the approach described in ipfs#115, define a new method signature on `Command` that supports full processing of the `Response` object when text encoding is requested. Add an encoding check and dispatch to DisplayCLI in local, http client, and http handler code paths. Unblocks resolution of `encoding` option processing in multiple go-ipfs issues. - ipfs/kubo#7050 json encoding for `ls` - ipfs/kubo#1121 json encoding for `add` - ipfs/kubo#5594 json encoding for `stats bw`

- When both PostRun (output transformer) and DisplayCLI (terminal output) are present, ensure that both are run. `EmitResponse` helper in executor.go is invoked by both local and HTTP client executors: the client fibs the command.Run interface via anonymous function.

executor.go

djdv · 2021-09-20T12:52:02Z

Sorry for the conflicts (；´∀｀)
Just as a heads up, feel free to keep or clobber whatever variable/function names were used in #216 and #217
The names don't matter compared to the fix itself, so the choice is yours when resolving the conflict.

Also, if this gets merged #219
it'll introduce another one. In cli.Run for this line if encType == cmds.Text && !cmd.HasText() {
If the linked PR is correct, you should be able to just drop the conflicting hunk in this PR when that happens.
(No guarantees that I'm right on that though, or that it gets merged in - but if so I see that you already handle this in cli.Parse too if req.Command.HasText() { )
^ Not worth it.

I'll try not to create any more trouble ;^)

lidel

Resolved conflicts. ✨

It seems #215 (comment) the only remaining question / concern – @jbouwman mind taking a look at @Stebalien's question?

Close the channel immediately if it will never be used.

jbouwman · 2021-09-23T20:55:34Z

Resolved conflicts. sparkles

It seems #215 (comment) the only remaining question / concern – @jbouwman mind taking a look at @Stebalien's question?

@lidel sure thing, done. @Stebalien does that look right, in terms of order of error checks?

executor.go

Stebalien · 2021-09-23T21:28:33Z

does that look right, in terms of order of error checks?

Yes. But I missed the other thing.

jbouwman · 2021-09-30T17:19:34Z

does that look right, in terms of order of error checks?

Yes. But I missed the other thing.

I had cycles to look at this again. The cli.ResponseEmitter wrapper for DisplayCLI seems a little odd -- its main role seems to be in allowing PostRun-enabled commands access to process exit value via a cast. I didn't see a less invasive way to do it though. I'd welcome your thoughts.

Stebalien

Let me think about this a bit.

Stebalien · 2021-10-05T21:46:39Z

cli/responseemitter.go

+	stdout io.Writer
+	stderr io.Writer
+
+	re   cmds.ResponseEmitter


You can embed this instead of manually forwarding.

Stebalien · 2021-10-05T21:59:16Z

cli/responseemitter.go

+		go func() {
+			err := req.Command.DisplayCLI(res, os.Stdout, os.Stderr)
+			if err != nil {
+				dre.CloseWithError(err)


Hm. This isn't going to work:

The caller will send an error to dre via CloseWithError.

DisplayCLI will get this error from res, and return it.

We're then going to feed it back into dre here.

Really, we need to expose this error via some method and/or a channel. I'll try to think of the right approach.

Stebalien · 2021-10-05T21:59:42Z

cli/responseemitter.go

+func (dre *displayResponseEmitter) SetLength(l uint64) {
+	dre.re.SetLength(l)
+}
+
+func (dre *displayResponseEmitter) SetStatus(code int) {
+	dre.l.Lock()
+	defer dre.l.Unlock()
+	dre.exit = code
+}
+
+func (dre *displayResponseEmitter) Status() int {
+	dre.l.Lock()
+	defer dre.l.Unlock()
+	return dre.exit
+}
+
+func (dre *displayResponseEmitter) Stderr() io.Writer {
+	return dre.stderr
+}
+
+func (dre *displayResponseEmitter) Stdout() io.Writer {
+	return dre.stdout
+}
+


I think we can actually just drop these methods from the interface. But it'll require some investigation.

BigLep · 2021-11-12T16:32:44Z

@Stebalien : what are the recommended next steps? Is this high enough priority for us to engage on right now?

BigLep · 2022-03-03T13:45:46Z

@jbouwman : it looks like there are some comments from @Stebalien that you can incorporate (but I see there are open questions too).

@Stebalien : are you able to give definitive on what should be done here, or should we drop this for now due to not having bandwidth to engage?

BigLep · 2022-03-10T07:58:51Z

@jbouwman : I have converted this back to draft. Please mark it as "Ready for review" when comments have been incorporated.

Stebalien · 2022-03-22T21:33:19Z

are you able to give definitive on what should be done here, or should we drop this for now due to not having bandwidth to engage?

I just don't have the bandwidth right now, unfortunately.

This comment has been minimized.

Sign in to view

jbouwman mentioned this pull request Aug 19, 2021

DisplayCLI for stat bw CLI command ipfs/kubo#8358

Draft

jbouwman force-pushed the display-cli branch from 7ee6142 to 6782607 Compare August 21, 2021 18:55

jbouwman marked this pull request as ready for review August 21, 2021 18:58

BigLep requested a review from lidel September 3, 2021 15:12

Stebalien requested changes Sep 7, 2021

View reviewed changes

executor.go Outdated Show resolved Hide resolved

http/responseemitter.go Outdated Show resolved Hide resolved

http/client.go Outdated Show resolved Hide resolved

executor.go Outdated Show resolved Hide resolved

jbouwman added 2 commits September 8, 2021 12:08

revert local encoder

16f14e9

jbouwman force-pushed the display-cli branch from 979b1bd to 9e5e502 Compare September 8, 2021 19:59

jbouwman force-pushed the display-cli branch from 9e5e502 to 11083f0 Compare September 8, 2021 20:07

Run gofmt

9bff5f3

Stebalien reviewed Sep 9, 2021

View reviewed changes

executor.go Show resolved Hide resolved

Merge branch 'master' into display-cli

ed1225f

lidel reviewed Sep 23, 2021

View reviewed changes

lidel added the need/author-input Needs input from the original author label Sep 23, 2021

Read from display error channel in EmitResponse

f47dce6

Close the channel immediately if it will never be used.

Stebalien reviewed Sep 23, 2021

View reviewed changes

executor.go Show resolved Hide resolved

Relocate DisplayCLI logic from executor to cli ResponseEmitter

d3455df

Don't ignore the error returned by DisplayCLI

bce8c65

BigLep requested a review from Stebalien October 1, 2021 15:15

BigLep removed the need/author-input Needs input from the original author label Oct 1, 2021

Stebalien reviewed Oct 5, 2021

View reviewed changes

aschmahmann added need/analysis Needs further analysis before proceeding kind/enhancement A net-new feature or improvement to an existing feature labels Oct 8, 2021

aschmahmann assigned Stebalien Oct 8, 2021

BigLep requested a review from Stebalien March 3, 2022 13:44

BigLep assigned jbouwman and unassigned Stebalien Mar 3, 2022

BigLep mentioned this pull request Mar 3, 2022

Document global flags on sub-commands ipfs/kubo#6640

Open

BigLep added this to the Best Effort Track milestone Mar 10, 2022

BigLep marked this pull request as draft March 10, 2022 07:58

Merge branch 'ipfs:master' into display-cli

11a3754

BigLep added need/author-input Needs input from the original author and removed need/analysis Needs further analysis before proceeding labels May 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLI and API command output encoding #215

CLI and API command output encoding #215

jbouwman commented Aug 19, 2021 •

edited by lidel

Loading

This comment has been minimized.

Stebalien left a comment

Stebalien commented Sep 7, 2021

jbouwman commented Sep 7, 2021

djdv commented Sep 20, 2021 •

edited

Loading

lidel left a comment •

edited

Loading

jbouwman commented Sep 23, 2021

Stebalien commented Sep 23, 2021

jbouwman commented Sep 30, 2021

Stebalien left a comment

Stebalien Oct 5, 2021

Stebalien Oct 5, 2021

Stebalien Oct 5, 2021

BigLep commented Nov 12, 2021

BigLep commented Mar 3, 2022

BigLep commented Mar 10, 2022

Stebalien commented Mar 22, 2022

CLI and API command output encoding #215

Are you sure you want to change the base?

CLI and API command output encoding #215

Conversation

jbouwman commented Aug 19, 2021 • edited by lidel Loading

This comment has been minimized.

Stebalien left a comment

Choose a reason for hiding this comment

Stebalien commented Sep 7, 2021

jbouwman commented Sep 7, 2021

djdv commented Sep 20, 2021 • edited Loading

lidel left a comment • edited Loading

Choose a reason for hiding this comment

jbouwman commented Sep 23, 2021

Stebalien commented Sep 23, 2021

jbouwman commented Sep 30, 2021

Stebalien left a comment

Choose a reason for hiding this comment

Stebalien Oct 5, 2021

Choose a reason for hiding this comment

Stebalien Oct 5, 2021

Choose a reason for hiding this comment

Stebalien Oct 5, 2021

Choose a reason for hiding this comment

BigLep commented Nov 12, 2021

BigLep commented Mar 3, 2022

BigLep commented Mar 10, 2022

Stebalien commented Mar 22, 2022

jbouwman commented Aug 19, 2021 •

edited by lidel

Loading

djdv commented Sep 20, 2021 •

edited

Loading

lidel left a comment •

edited

Loading