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

Better key output #886

Merged
merged 2 commits into from
Apr 20, 2018
Merged

Better key output #886

merged 2 commits into from
Apr 20, 2018

Conversation

rigelrozanski
Copy link
Contributor

@rigelrozanski rigelrozanski commented Apr 20, 2018

Better key output:

gaiacli keys show foo

NAME:	ADDRESS:					PUBKEY:
foo	A954D44A8C703C3367AA90043D9FA1817E4D36B4	1624DE6220C1F063AEEF411280F0C30AB0F2325555E5FA6A4D66E2752282797DDA3AA3B270

gaiacli keys show foo --output=json

{
	"name": "foo",
	"address": "A954D44A8C703C3367AA90043D9FA1817E4D36B4",
	"pub_key": "1624DE6220C1F063AEEF411280F0C30AB0F2325555E5FA6A4D66E2752282797DDA3AA3B270"
}
  • Update docs
  • Updated all code comments where relevant
  • Updated CHANGELOG.md

@rigelrozanski rigelrozanski requested a review from ebuchman as a code owner April 20, 2018 16:54
@rigelrozanski rigelrozanski changed the base branch from master to develop April 20, 2018 16:54
@codecov
Copy link

codecov bot commented Apr 20, 2018

Codecov Report

Merging #886 into develop will not change coverage.
The diff coverage is 0%.

@@           Coverage Diff            @@
##           develop     #886   +/-   ##
========================================
  Coverage    67.01%   67.01%           
========================================
  Files           68       68           
  Lines         3614     3614           
========================================
  Hits          2422     2422           
  Misses        1008     1008           
  Partials       184      184

updated docs for new key work
@rigelrozanski rigelrozanski requested a review from cwgoes April 20, 2018 17:17
@rigelrozanski rigelrozanski changed the title Rigel/better key output Better key output Apr 20, 2018
Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

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

Tested ACK

I like the output flag, we should make that standard on any command that should be both human & machine readable - which should be all commands?

If tabular outputs are common, we could use a library - e.g. https://github.com/olekukonko/tablewriter - instead of manual '\t' characters.

@rigelrozanski
Copy link
Contributor Author

rigelrozanski commented Apr 20, 2018

@cwgoes thanks! Side comment - unless a library is doing something otherwise pretty unique and important its better for use to not add an extra import and add some more code internally - the big risk is that malicious code gets added to another library and is somehow able to infiltrate / introduce security holes in the cosmos codebase without anyone noticing - I think something as basic as the use of \t shouldn't require an import

@rigelrozanski rigelrozanski merged commit d613c2b into develop Apr 20, 2018
@rigelrozanski rigelrozanski deleted the rigel/better-key-output branch April 20, 2018 21:17
@cwgoes
Copy link
Contributor

cwgoes commented Apr 20, 2018

Could be mitigated by version pinning libraries (we'll never need to update the tabulation library) - but agreed in general.

@jaekwon
Copy link
Contributor

jaekwon commented Apr 21, 2018

We're supposed to migrate to Bech32Cosmos. #263

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.

3 participants