-
Notifications
You must be signed in to change notification settings - Fork 151
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
Support for deserializing params *object* as single first parameter #347
Conversation
Codecov Report
@@ Coverage Diff @@
## master #347 +/- ##
==========================================
- Coverage 90.56% 87.13% -3.44%
==========================================
Files 37 37
Lines 2438 2464 +26
Branches 0 484 +484
==========================================
- Hits 2208 2147 -61
- Misses 230 232 +2
- Partials 0 85 +85
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exciting to see this getting done! And thanks for writing the tests.
…r-deserializing-params-object-as-single-first-parameter
src/StreamJsonRpc/JsonRpc.cs
Outdated
/// <summary> | ||
/// Tries to obtain the <see cref="MethodSignatureAndTarget"/> info of a method. | ||
/// </summary> | ||
internal bool MethodSupportsSingleParameterObjectDeserialization(string methodName, Type parameterType) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Being internal means this magic trick isn't reproducible by other (external) formatters. I like that it's your default, but I'd like to explore whether we can make the functionality available publicly.
Your method as it stands is very focused on your feature. If however we change it to simply return the JsonRpcMethodAttribute
for any RPC method, then it will likely be useful not just for this but other future features, which makes me feel better about making it public.
I'm going to play around with this idea and may push a commit to your PR to see what you think, if that's OK with you.
This generalizes the new method on JsonRpc and makes it public.
…ibute This gives per-method callers the ability to customize how their RPC method is invoked using all the power of the attribute. It also adds a default constructor to the attribute so its more generally useful now that it has more functionality.
@milopezc Now that I've pushed a few commits to this PR, can you review as well? If you approve too, we'll complete this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good, github is not letting me select the Approve option though.
Closes #289