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

Implement Tx Decode in REST/CLI Clients #4026

Closed
wants to merge 10 commits into from
Closed

Implement Tx Decode in REST/CLI Clients #4026

wants to merge 10 commits into from

Conversation

yangyanqing
Copy link
Contributor

@yangyanqing yangyanqing commented Apr 2, 2019 •
edited
Loading

close: #3872

  • 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: sdkch 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)

alexanderbez
alexanderbez reviewed Apr 2, 2019
View reviewed changes
client/tx/decode.go Show resolved Hide resolved
client/tx/decode.go Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Apr 2, 2019

Codecov Report

Merging #4026 into develop will increase coverage by 0.01%.
The diff coverage is 0%.

@@             Coverage Diff             @@
##           develop    #4026      +/-   ##
===========================================
+ Coverage    60.01%   60.02%   +0.01%     
===========================================
  Files          212      212              
  Lines        15104    15108       +4     
===========================================
+ Hits          9064     9068       +4     
  Misses        5419     5419              
  Partials       621      621

@codecov
Copy link

codecov bot commented Apr 2, 2019 •
edited
Loading

Codecov Report

Merging #4026 into develop will decrease coverage by 0.04%.
The diff coverage is 0%.

@@             Coverage Diff             @@
##           develop    #4026      +/-   ##
===========================================
- Coverage     60.2%   60.16%   -0.05%     
===========================================
  Files          212      212              
  Lines        15157    15161       +4     
===========================================
- Hits          9126     9122       -4     
- Misses        5407     5415       +8     
  Partials       624      624

@fedekunze fedekunze requested a review from sabau April 2, 2019 23:21
@yangyanqing yangyanqing changed the title WIP: gaiacli tx decode #3872 R4R: gaiacli tx decode #3872 Apr 3, 2019
@yangyanqing
Copy link
Contributor Author

Could you please review my PR ?
@jackzampolin @alessio

@alexanderbez alexanderbez changed the title R4R: gaiacli tx decode #3872 gaiacli tx decode Apr 17, 2019
@alexanderbez alexanderbez changed the title gaiacli tx decode Implement Tx Decode in REST/CLI Clients Apr 17, 2019
alessio
alessio suggested changes Apr 17, 2019
View reviewed changes
Copy link
Contributor

@alessio alessio left a comment

Choose a reason for hiding this comment

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

Small change required

cmd/gaia/cli_test/test_helpers.go Outdated Show resolved Hide resolved
alexanderbez
alexanderbez suggested changes Apr 17, 2019
View reviewed changes
client/tx/decode.go Outdated Show resolved Hide resolved
alexanderbez
alexanderbez approved these changes Apr 18, 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.

One minor nit, but otherwise LGTM. Thanks @yangyanqing

.pending/features/gaiacli/3872-Add-gaiacli-tx- Outdated Show resolved Hide resolved
.pending/features/gaiarest/3872-Add-POST-txs-de Outdated Show resolved Hide resolved
@yangyanqing
Copy link
Contributor Author

One minor nit, but otherwise LGTM. Thanks @yangyanqing

Thanks for your correction ! @alexanderbez

alexanderbez
alexanderbez suggested changes Apr 18, 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.

Fails TestDecodeTx now.

@yangyanqing
Copy link
Contributor Author

Fails TestDecodeTx now.

Sorry, It's my fail !

Now, all tests was passed except codecov. It seems that codecov had some problems. Because the codes which was not covered are not modified in this PR.

BTW: Output of make ci may be improved to show error messages only. Info message should be as less as possible, debug message should not appear in stdout except some flag was set.

I like the style of cpputest very much. Each dot represents one case passed. Anybody can catch the failure easily.

image

@alexanderbez
Copy link
Contributor

@alessio and @fedekunze, do you mind reviewing this?

@ebuchman ebuchman closed this Apr 26, 2019
@yangyanqing yangyanqing mentioned this pull request Apr 27, 2019
Closed
5 tasks
@yangyanqing yangyanqing deleted the frank/3872-gaiacli-tx-decode branch April 27, 2019 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexanderbez alexanderbez alexanderbez approved these changes

@cwgoes cwgoes Awaiting requested review from cwgoes

@ebuchman ebuchman Awaiting requested review from ebuchman

@rigelrozanski rigelrozanski Awaiting requested review from rigelrozanski

@sabau sabau Awaiting requested review from sabau

@jackzampolin jackzampolin Awaiting requested review from jackzampolin

@alessio alessio Awaiting requested review from alessio

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
@yangyanqing @alexanderbez @alessio @ebuchman