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 params member to JSON-RPC request #97

Merged
merged 2 commits into from
Sep 9, 2021

Conversation

ogenev
Copy link
Member

@ogenev ogenev commented Sep 7, 2021

Adds support for parsing parameters for the JSON-RPC call.

Fixes #96

Copy link
Collaborator

@njgheorghita njgheorghita left a comment

Choose a reason for hiding this comment

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

Looks good! I love the test - but it seems like a test that could benefit from parameterization. I'm just bringing it up now since I've run into a couple similar situations on my local branches. From what I can tell, rstest is probably the best crate to use for various testing utils. Even if it's not necessary for this test, it seems like a crate we'll want to use in other places as the project grows.

@ogenev ogenev force-pushed the support-json-rpc-params-member branch from ed88f71 to aae16cb Compare September 7, 2021 18:37
@ogenev
Copy link
Member Author

ogenev commented Sep 8, 2021

Looks good! I love the test - but it seems like a test that could benefit from parameterization. I'm just bringing it up now since I've run into a couple similar situations on my local branches. From what I can tell, rstest is probably the best crate to use for various testing utils. Even if it's not necessary for this test, it seems like a crate we'll want to use in other places as the project grows.

Let me tell you a secret, I stole this test from one json-rpc crate 😄 Good to know about rstest, I tried it, but in this case, it requires to add more boilerplate code which I think will look confusing.

@njgheorghita
Copy link
Collaborator

I tried it, but in this case, it requires to add more boilerplate code which I think will look confusing.

Fair enough, but I have a feeling like we'll need it sooner or later. Also, it's often worthwhile to add some code complexity if it means improved / more granular tests. Obviously there's a limit somewhere. I don't think it's a blocker for merging this, but you might think about leaving a comment that these tests should be expanded at some point

@ogenev ogenev force-pushed the support-json-rpc-params-member branch from aae16cb to e1423f1 Compare September 8, 2021 21:11
@ogenev
Copy link
Member Author

ogenev commented Sep 8, 2021

Fair enough, but I have a feeling like we'll need it sooner or later. Also, it's often worthwhile to add some code complexity if it means improved / more granular tests. Obviously there's a limit somewhere. I don't think it's a blocker for merging this, but you might think about leaving a comment that these tests should be expanded at some point

Probably you are right, updated the test with parametrization.

@ogenev ogenev force-pushed the support-json-rpc-params-member branch 2 times, most recently from 6d2ad4a to 3ba4de1 Compare September 8, 2021 21:17
@ogenev ogenev force-pushed the support-json-rpc-params-member branch from 3ba4de1 to 4783277 Compare September 9, 2021 08:25
@njgheorghita
Copy link
Collaborator

@ogenev Looks great! Thanks for updating those tests. lgtm!

@ogenev ogenev merged commit 5f75c05 into ethereum:master Sep 9, 2021
@ogenev ogenev deleted the support-json-rpc-params-member branch September 9, 2021 15:14
# 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.

Support JSON-RPC params member
2 participants