Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add new definition for sdk.Router, add WithRouter to the BaseApp #5458

Closed
wants to merge 3 commits into from

Conversation

PumpkinSeed
Copy link
Contributor

@PumpkinSeed PumpkinSeed commented Jan 1, 2020

Closes: #5455

Description

This PR makes the baseapp more generic. Add custom Router definitions opening it up for stateful routers.


For contributor use:

  • 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.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@codecov
Copy link

codecov bot commented Jan 1, 2020

Codecov Report

Merging #5458 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5458      +/-   ##
==========================================
+ Coverage   54.38%   54.39%   +<.01%     
==========================================
  Files         313      313              
  Lines       18836    18838       +2     
==========================================
+ Hits        10244    10246       +2     
  Misses       7807     7807              
  Partials      785      785
Impacted Files Coverage Δ
baseapp/baseapp.go 71.58% <100%> (+0.21%) ⬆️
baseapp/router.go 100% <100%> (ø) ⬆️

Co-Authored-By: Bot from GolangCI <42910462+golangcibot@users.noreply.github.com>
@sunnya97
Copy link
Member

sunnya97 commented Jan 1, 2020

Thanks @PumpkinSeed! I actually had a finished PR for this already. #5452

Sorry, should have linked to it in the issue. Do you mind pointing the PR to the branch ctx_msg_router, so we can merge in the test you wrote?

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.

Thanks @PumpkinSeed! As @sunnya97 stated, this should be closed or pushed to @sunnya97's branch to include the additional tests you wrote. Thanks for the contributions!

@PumpkinSeed
Copy link
Contributor Author

As discussion above.

@PumpkinSeed PumpkinSeed closed this Jan 2, 2020
@PumpkinSeed PumpkinSeed mentioned this pull request Jan 2, 2020
11 tasks
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sdk.Router should be stateful
4 participants