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

Added UseSingleObjectParameterDeserialization option to JsonRpcTargetOptions #446

Merged
merged 1 commit into from
Apr 11, 2020
Merged

Added UseSingleObjectParameterDeserialization option to JsonRpcTargetOptions #446

merged 1 commit into from
Apr 11, 2020

Conversation

daniel-vetter
Copy link
Contributor

I added the the UseSingleObjectParameterDeserialization property to the target options. When the property is set to true, the GetRequestMethodToClrMethodMap method will add a JsonRpcMethodAttribute to the MethodSignatureAndTarget. I don't know if this is the correct approach because GetJsonRpcMethodAttribute will now return a attribute, that did not exist on the original method. But it seems like AddLocalRpcMethod is also creating fake attributes, so i think it should be ok.

resolves #444

@msftclas
Copy link

msftclas commented Apr 5, 2020

CLA assistant check
All CLA requirements met.

Copy link
Member

@AArnott AArnott left a comment

Choose a reason for hiding this comment

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

Thanks for your submission. I look forward to merging when the issues I call out are corrected.

src/StreamJsonRpc/JsonRpc.cs Show resolved Hide resolved
src/StreamJsonRpc/JsonRpcTargetOptions.cs Outdated Show resolved Hide resolved
src/StreamJsonRpc.Tests/JsonRpcTests.cs Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Apr 6, 2020

Codecov Report

Merging #446 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #446   +/-   ##
=======================================
  Coverage   90.55%   90.56%           
=======================================
  Files          49       49           
  Lines        3769     3772    +3     
=======================================
+ Hits         3413     3416    +3     
  Misses        356      356           
Impacted Files Coverage Δ
src/StreamJsonRpc/JsonRpc.cs 92.84% <100.00%> (+0.01%) ⬆️
src/StreamJsonRpc/JsonRpcTargetOptions.cs 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 01740bb...181b775. Read the comment docs.

Copy link
Member

@AArnott AArnott left a comment

Choose a reason for hiding this comment

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

Thank you!

@AArnott
Copy link
Member

AArnott commented Apr 10, 2020

@daniel-vetter As there are 6 commits for this relatively small change, do you mind if I squash when completing this PR? Or would you like to squash your own commits and do a force push to your branch before I complete it?

@daniel-vetter
Copy link
Contributor Author

@AArnott I squashed everything. You can complete the PR.

@AArnott AArnott added this to the v2.5 milestone Apr 11, 2020
@AArnott AArnott merged commit a92a423 into microsoft:master Apr 11, 2020
@daniel-vetter daniel-vetter deleted the daniel-vetter/centralized-single-object-parameter-deserialization-option branch April 19, 2020 22:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UseSingleObjectParameterDeserialization without adding a attribute to all methods
4 participants