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.

Sign up for GitHub

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

Jump to bottom

Remove client/input.{Buffer,Override}Stdin() functions #4602

Merged
merged 1 commit into from
Jun 22, 2019
Merged

Remove client/input.{Buffer,Override}Stdin() functions #4602

merged 1 commit into from
Jun 22, 2019

Conversation

alessio
Copy link
Contributor

@alessio alessio commented Jun 20, 2019 •
edited by alexanderbez
Loading

Thanks: @jleni for the patch.

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
  • Wrote tests
  • Updated relevant documentation (docs/)
  • Added a relevant changelog entry: clog add [section] [stanza] [message]
  • rereviewed Files changed in the github PR explorer

For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@alessio alessio changed the title Alessio/upgrade cobra Upgrade cobra to latest release Jun 20, 2019
@codecov
Copy link

codecov bot commented Jun 20, 2019

Codecov Report

Merging #4602 into master will decrease coverage by 0.01%.
The diff coverage is 57.57%.

@@            Coverage Diff             @@
##           master    #4602      +/-   ##
==========================================
- Coverage   52.79%   52.78%   -0.02%     
==========================================
  Files         264      264              
  Lines       16468    16466       -2     
==========================================
- Hits         8695     8691       -4     
- Misses       7126     7128       +2     
  Partials      647      647

@alessio
Remove client/input.{Buffer,Override}Stdin() functions
2f09daa 
Cobra's new release made them redundant.

Thanks: Juan Leni <juan.leni@zondax.ch> for the original patch.
@alessio alessio changed the title Upgrade cobra to latest release Remove client/input.{Buffer,Override}Stdin() functions Jun 20, 2019
@alessio alessio marked this pull request as ready for review June 20, 2019 14:58
alexanderbez
alexanderbez reviewed Jun 20, 2019
View reviewed changes
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

ACK -- Can you quickly test/play with this on Gaia @alessio ?

sabau
sabau approved these changes Jun 21, 2019
View reviewed changes
Copy link
Contributor

@sabau sabau left a comment

Choose a reason for hiding this comment

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

LGTM is it now happy on gaia?

@alessio
Copy link
Contributor Author

alessio commented Jun 21, 2019

No, it isn't. There is one particular test case (TestGaiaCLIConfirmTx) that needs rewriting. I reckon it does not make sense to block this PR because of that, considering that all cli tests will have to be rewritten anyway.

@sabau
Copy link
Contributor

sabau commented Jun 21, 2019

Ok, we can open a PR/issue on gaia related to this

alessio added a commit to cosmos/gaia that referenced this pull request Jun 21, 2019
@alessio
Testing cosmos/cosmos-sdk#4602
721466f
@alessio
Copy link
Contributor Author

alessio commented Jun 21, 2019

Tests pass now: cosmos/gaia#52

fedekunze
fedekunze reviewed Jun 21, 2019
View reviewed changes
go.sum
@@ -122,19 +128,22 @@ github.com/rcrowley/go-metrics v0.0.0-20180503174638-e2704e165165 h1:nkcn14uNmFE
github.com/rcrowley/go-metrics v0.0.0-20180503174638-e2704e165165/go.mod h1:bCqnVzQkZxMG4s8nGwiZ5l3QUCyqpo9Y+/ZMZ9VjZe4=
github.com/rs/cors v1.6.0 h1:G9tHG9lebljV9mfp9SNPDL36nCDxmo3zTlAf1YgvzmI=
github.com/rs/cors v1.6.0/go.mod h1:gFx+x8UowdsKA9AchylcLynDq+nNFfI8FkUZdN/jGCU=
github.com/russross/blackfriday v1.5.2/go.mod h1:JO/DiYxRf+HjHt06OyowR9PTA263kcR/rfWxYHBV53g=
Copy link
Collaborator

Choose a reason for hiding this comment

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

?

go.sum
@@ -6,6 +6,7 @@ github.com/VividCortex/gohistogram v1.0.0/go.mod h1:Pf5mBqqDxYaXu3hDrrU+w6nw50o/
github.com/aead/siphash v1.0.1/go.mod h1:Nywa3cDsYNNK3gaciGTWPwHt0wlpNV15vwmswBAUSII=
github.com/alecthomas/template v0.0.0-20160405071501-a0175ee3bccc/go.mod h1:LOuyumcjzFXgccqObfd/Ljyb9UuFJ6TxHnclSeseNhc=
github.com/alecthomas/units v0.0.0-20151022065526-2efee857e7cf/go.mod h1:ybxpYRFXyAe+OPACYpWeL0wqObRcbAqCMya13uyzqw0=
github.com/armon/consul-api v0.0.0-20180202201655-eb2c6b5be1b6/go.mod h1:grANhF5doyWs3UAsr3K4I6qtAmlQcZDesFNEHPZAzj8=
Copy link
Collaborator

Choose a reason for hiding this comment

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

?

go.sum
github.com/spf13/cobra v0.0.3/go.mod h1:1l0Ry5zgKvJasoi3XT1TypsSe7PqH0Sj9dhYf7v3XqQ=
github.com/spf13/cobra v0.0.5 h1:f0B+LkLX6DtmRH1isoNA9VTtNUK9K8xYd28JNNfOv/s=
github.com/spf13/cobra v0.0.5/go.mod h1:3K3wKZymM7VvHMDS9+Akkh4K60UwM26emMESw8tLCHU=
github.com/spf13/jwalterweatherman v1.0.0/go.mod h1:cQK4TGJAtQXfYWX+Ddv3mKDzgVb68N+wFjFa4jdeBTo=
Copy link
Collaborator

Choose a reason for hiding this comment

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

?

go.sum
@@ -153,6 +162,8 @@ github.com/tendermint/iavl v0.12.2 h1:Ls5p5VINCM1HRT9g5Vvs2zmDOCU/CCIvIHzd/pZ8P0
github.com/tendermint/iavl v0.12.2/go.mod h1:EoKMMv++tDOL5qKKVnoIqtVPshRrEPeJ0WsgDOLAauM=
github.com/tendermint/tendermint v0.31.5 h1:vTet8tCq3B9/J9Yo11dNZ8pOB7NtSy++bVSfkP4KzR4=
github.com/tendermint/tendermint v0.31.5/go.mod h1:ymcPyWblXCplCPQjbOYbrF1fWnpslATMVqiGgWbZrlc=
github.com/ugorji/go/codec v0.0.0-20181204163529-d75b2dcb6bc8/go.mod h1:VFNgLljTbGfSG7qAOspJ7OScBnGdDN/yBr0sguwnwf0=
Copy link
Collaborator

Choose a reason for hiding this comment

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

?

go.sum
@@ -153,6 +162,8 @@ github.com/tendermint/iavl v0.12.2 h1:Ls5p5VINCM1HRT9g5Vvs2zmDOCU/CCIvIHzd/pZ8P0
github.com/tendermint/iavl v0.12.2/go.mod h1:EoKMMv++tDOL5qKKVnoIqtVPshRrEPeJ0WsgDOLAauM=
github.com/tendermint/tendermint v0.31.5 h1:vTet8tCq3B9/J9Yo11dNZ8pOB7NtSy++bVSfkP4KzR4=
github.com/tendermint/tendermint v0.31.5/go.mod h1:ymcPyWblXCplCPQjbOYbrF1fWnpslATMVqiGgWbZrlc=
github.com/ugorji/go/codec v0.0.0-20181204163529-d75b2dcb6bc8/go.mod h1:VFNgLljTbGfSG7qAOspJ7OScBnGdDN/yBr0sguwnwf0=
github.com/xordataexchange/crypt v0.0.3-0.20170626215501-b2862e3d0a77/go.mod h1:aYKd//L2LvnjZzWKhF00oedf4jCCReLcmhLdhm1A27Q=
Copy link
Collaborator

Choose a reason for hiding this comment

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

?

@alessio
Copy link
Contributor Author

alessio commented Jun 21, 2019

@fedekunze it appears that the new cobra depends on a bunch of additional packages. Can't do much to address your (legimitate) concerns

@alessio
Copy link
Contributor Author

alessio commented Jun 21, 2019

...other than reviewing the additional deps, of course

alexanderbez
alexanderbez approved these changes Jun 21, 2019
View reviewed changes
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

ACK

@alessio alessio merged commit 7b5e6ce into master Jun 22, 2019
@alessio alessio deleted the alessio/upgrade-cobra branch June 22, 2019 09:25
@alessio alessio mentioned this pull request Jun 22, 2019
Merged
5 tasks
alessio pushed a commit to cosmos/gaia that referenced this pull request Jun 24, 2019
upgrade to latest sdk (#52)
0c12cce 
As per changes introduced with cosmos/cosmos-sdk#4602
bchainexpt pushed a commit to bchainexpt/Blockchain-wallet-andorid-app that referenced this pull request Apr 4, 2022
upgrade to latest sdk (#52)
51bf887 
As per changes introduced with cosmos/cosmos-sdk#4602
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fedekunze fedekunze fedekunze left review comments

@sabau sabau sabau approved these changes

@alexanderbez alexanderbez alexanderbez approved these changes

@jackzampolin jackzampolin Awaiting requested review from jackzampolin

@rigelrozanski rigelrozanski Awaiting requested review from rigelrozanski

@jleni jleni Awaiting requested review from jleni

Assignees
No one assigned
Labels
C:CLI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@alessio @sabau @alexanderbez @fedekunze