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

Port weave errors package #4821

Merged
merged 10 commits into from
Jul 31, 2019
Merged

Conversation

ethanfrey
Copy link
Contributor

@ethanfrey ethanfrey commented Jul 31, 2019

Addresses #4708 after some discussion with @marbar3778 on how the core team would like errors handled.

This introduces the basic components from the weave/errors package. It adds codespace support, and Registers the same error codes and messages as defined in cosmos-sdk/types/errors.go. Stack trace and reporting formatting is directly taken from the weave codebase.

This is a minimal package without any changes to existing code, for ease of merging. Please play with the code (add tests and print out stack traces, etc). I am happy to make any adjustments as needed for the cosmos-sdk format. I am also happy to add multierror (so we can return a list of validation errors in one pass) if it is desired.

I would like to merge this package with any adjustments as one PR, before opening another one that modifies existing modules or interfaces (and thus would be after v0.36 final)

  • Targeted PR against correct branch (see CONTRIBUTING.md)

  • 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] [-t <tag>] [-m <msg>]

  • Re-reviewed 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)

errors/abci.go Outdated Show resolved Hide resolved
@ethanfrey ethanfrey force-pushed the confio/errors-package branch from d42fb92 to 89f4afa Compare July 31, 2019 12:14
@codecov
Copy link

codecov bot commented Jul 31, 2019

Codecov Report

Merging #4821 into master will increase coverage by 0.25%.
The diff coverage is 79.87%.

@@            Coverage Diff             @@
##           master    #4821      +/-   ##
==========================================
+ Coverage    50.5%   50.76%   +0.25%     
==========================================
  Files         288      291       +3     
  Lines       18516    18680     +164     
==========================================
+ Hits         9351     9482     +131     
- Misses       8480     8504      +24     
- Partials      685      694       +9

@ethanfrey ethanfrey force-pushed the confio/errors-package branch from a9b7a98 to 43b255b Compare July 31, 2019 12:51
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.

Can we move all the new components into types/errors/ instead? Also, I think a pending log entry here would be beneficial too. Otherwise, LGTM.

@ethanfrey
Copy link
Contributor Author

Thanks for the feedback, I will update this.

Does the API need more docs?

@alexanderbez
Copy link
Contributor

No I think this is great! Thanks @ethanfrey :)

Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

💯 ACK

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 ⚡️

@alexanderbez alexanderbez merged commit 68dd969 into cosmos:master Jul 31, 2019
@ethanfrey
Copy link
Contributor Author

Fast as greased lightning!

Thanks @alexanderbez

@ethanfrey ethanfrey deleted the confio/errors-package branch July 31, 2019 16:37
@ethanfrey ethanfrey mentioned this pull request Aug 5, 2019
4 tasks
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.

4 participants