Skip to content

[rpc] transform /status result.node_info.other into map#2417

Merged
xla merged 2 commits intodevelopfrom
2391-node-info-other
Sep 17, 2018
Merged

[rpc] transform /status result.node_info.other into map#2417
xla merged 2 commits intodevelopfrom
2391-node-info-other

Conversation

@melekes
Copy link
Contributor

@melekes melekes commented Sep 17, 2018

Refs #2391

  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Updated CHANGELOG_PENDING.md

@melekes melekes force-pushed the 2391-node-info-other branch from 2ce1abc to a4e38a8 Compare September 17, 2018 11:43
@melekes melekes requested a review from zramsay as a code owner September 17, 2018 13:02
@melekes melekes force-pushed the 2391-node-info-other branch from 1d5634c to f7fd422 Compare September 17, 2018 13:04
@xla xla added C:rpc Component: JSON RPC, gRPC T:enhancement Type: Enhancement labels Sep 17, 2018
Copy link
Contributor

@xla xla left a comment

Choose a reason for hiding this comment

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

👍 :octocat: :shipit: 🍡

LGTM, yet it seems we while we touch that part of the response we could rethink how meaningful a bucket like Other is. By the looks of it there is a lot of version info in there, so might be worth breaking those out into something like Versions, which as hinted in the inline comment might be better addressed when the Versions changes come to full effect.

other.AminoVersion,
other.P2PVersion,
other.ConsensusVersion,
other.RPCVersion}
Copy link
Contributor

Choose a reason for hiding this comment

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

Inconsistent formatting, should be:

	versions := []string{
		other.AminoVersion,
		other.AminoVersion,
		other.P2PVersion,
		other.ConsensusVersion,
		other.RPCVersion,
	}

@melekes
Copy link
Contributor Author

melekes commented Sep 17, 2018

It's pity that amino does not support maps.

By the looks of it there is a lot of version info in there, so might be worth breaking those out into something like Versions

good idea 👍 but I think it should be easy to extract fields and really be done in versions PR

@xla xla merged commit fc7f9bc into develop Sep 17, 2018
@xla xla deleted the 2391-node-info-other branch September 17, 2018 16:39
melekes added a commit that referenced this pull request Sep 18, 2018
xla pushed a commit that referenced this pull request Sep 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C:rpc Component: JSON RPC, gRPC T:enhancement Type: Enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants