go-filecoin show and go-filecoin chain combined (#1587) #3106
Conversation
|
@shannonwells could you please shepherd this PR? |
deaswang
force-pushed
the
refactor/1587-combine-chain-and-show
branch
from
c42ace8 to
5bddcec
Compare
|
@shannonwells Any response for this PR? |
commands/chain_daemon_test.go
Outdated
| assert.Contains(t, output, "Height: 1") | ||
| assert.Contains(t, output, "Nonce: 0") | ||
| assert.Contains(t, output, "Timestamp: ") | ||
| assert.Contains(t, output, "Messages: ") |
There was a problem hiding this comment.
We need a test case for blocks that contain messages. The block being tested here doesn't have any messages.
commands/chain_daemon_test.go
Outdated
| }) | ||
| } | ||
|
|
||
| func TestBlockDaemon(t *testing.T) { |
There was a problem hiding this comment.
We want to avoid adding and more TestDaemon tests. Please write this test using the FAST Library. An example can be found here: https://github.com/filecoin-project/go-filecoin/blob/fbc487995e365de00b1986ed3a06cc411cb17409/commands/payment_channel_daemon_test.go#L21-L24
There was a problem hiding this comment.
Need all the chain_daemon_test use FAST Library? TestChainLs TestChainHead
|
|
||
| showMessages, _ := req.Options["messages"].(bool) | ||
| if showMessages == true { | ||
| _, err = fmt.Fprintf(w, `Messages: %s`+"\n", block.Messages) |
There was a problem hiding this comment.
I'm curious to know what this output would look like. I imagine it would not be very human-friendly, which defeats the purpose of a Text output encoder. Also, what happens when you combine showMessages=false and you use --enc json ?
There was a problem hiding this comment.
in text enc, if show message, there will be one extra line Messages: [], if not show message, no message line.
in json enc, the code will not run here. no matter show or not show message, it will be the marshall of types.Block, so messages":null always exist in json body.
about the messages format, it depend on fmt.Fprintf in text enc, json Marshall in json enc.
about enc, if you run go-filecoin --help, you'll find it is main option, so if something need update, it is not only chain command problem.
| }, | ||
| Type: types.Block{}, | ||
| Encoders: cmds.EncoderMap{ | ||
| cmds.Text: cmds.MakeTypedEncoder(func(req *cmds.Request, w io.Writer, block *types.Block) error { |
There was a problem hiding this comment.
this is copy from old show block command. and almost all the command has extra Text encoders. so this text encoders should be one new issue to fix all commands.
There was a problem hiding this comment.
You are not wrong, but there has been some discussion about not putting in new Text encodings and having people add | jq if they want human-readable output. Even so it's fine to leave it as written.
commands/chain_daemon_test.go
Outdated
| }) | ||
|
|
||
| t.Run("chain block <cid-of-genesis-block> --enc json returns JSON for a filecoin block", func(t *testing.T) { | ||
| d := th.NewDaemon(t, |
There was a problem hiding this comment.
Also need a test for combining --show-messages false and --enc json
There was a problem hiding this comment.
not support --show-messages false and --enc json. it always show message in json enc.
deaswang
force-pushed
the
refactor/1587-combine-chain-and-show
branch
from
bdc6628 to
753ac0c
Compare
deaswang
force-pushed
the
refactor/1587-combine-chain-and-show
branch
from
c2e4435 to
9f2c142
Compare
|
@frrist I have update the test use FAST Library. |
|
#3196 |
Fixes #1587