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

Simplify signature of handler install #960

Merged
merged 10 commits into from
Mar 17, 2020

Conversation

eddyashton
Copy link
Member

Resolves #914.

We have several parameters to tweak the behaviour of installed RPC handlers, and we're likely to add more. Rather than needing to specify all of these in the call to install (with signficant overload complications), they should be set manually on the few handlers they actually affect.

This PR lets you set those parameters in 2 ways. Manually:

auto& foo_handler = install("FOO", foo_func, Read);
foo_handler.execute_locally = true;
foo_handler.params_schema = ...;

Or with chaining methods:

install("FOO", foo_func, Read)
  .set_execute_locally(true)
  .set_params_schema(...);

I prefer the second, but I think its worth keeping both in general.

@eddyashton eddyashton requested a review from a team as a code owner March 17, 2020 15:11
@codecov-io
Copy link

Codecov Report

Merging #960 into master will decrease coverage by 0.05%.
The diff coverage is 95.29%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #960      +/-   ##
==========================================
- Coverage   68.13%   68.09%   -0.05%     
==========================================
  Files         103      103              
  Lines        8146     8147       +1     
==========================================
- Hits         5550     5547       -3     
- Misses       2596     2600       +4
Flag Coverage Δ
#unit_BFT 68.09% <95.29%> (-0.03%) ⬇️
#unit_CFT 68.07% <95.29%> (-0.05%) ⬇️
Impacted Files Coverage Δ
src/node/rpc/memberfrontend.h 67.41% <100%> (+0.43%) ⬆️
src/node/rpc/commonhandlerregistry.h 25.81% <100%> (-0.48%) ⬇️
src/node/rpc/userfrontend.h 100% <100%> (ø) ⬆️
src/node/rpc/frontend.h 64.77% <100%> (ø) ⬆️
src/node/rpc/nodefrontend.h 78.65% <100%> (ø) ⬆️
src/node/rpc/handlerregistry.h 82.98% <84%> (-9.33%) ⬇️

@ghost
Copy link

ghost commented Mar 17, 2020

simple_install_signature@6059 aka 20200317.11 vs master ewma over 30 builds from 5763 to 6053
images

@eddyashton eddyashton merged commit 487d9b5 into microsoft:master Mar 17, 2020
@eddyashton eddyashton deleted the simple_install_signature branch March 17, 2020 17:09
@achamayou
Copy link
Member

@eddyashton this is great! I think we should have the accessors only, and make sure they're exposed in doxygen/sphinx.

eddyashton added a commit to eddyashton/CCF that referenced this pull request Mar 24, 2020
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.

Simplify signature of handler install
5 participants