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

add json() impl to Request #204

Merged
merged 5 commits into from
Apr 6, 2022
Merged

add json() impl to Request #204

merged 5 commits into from
Apr 6, 2022

Conversation

minauteur
Copy link
Contributor

In draft until feedback on approach is given in issue #199 thread

@minauteur minauteur force-pushed the master branch 3 times, most recently from 456d277 to 0566caa Compare March 30, 2022 23:23
@ranile
Copy link
Collaborator

ranile commented Apr 2, 2022

Sorry, it took me a little bit to get to this. It looks good. You can go ahead and finish it up so it can be merged.

Also, if you could add a test case for this, that would be great

@minauteur
Copy link
Contributor Author

No worries!
I'll get this cleaned up and have tests in order so this can be ready for review sometime this evening or tomorrow morning.
Thanks!

@minauteur minauteur marked this pull request as ready for review April 4, 2022 00:35
@minauteur
Copy link
Contributor Author

minauteur commented Apr 4, 2022

I've cleaned up the formatting, and--for now--I've updated the test post_json() in crates/net/tests/http.rs to utilize the convenience method added in the PR--passes locally with HTTPBIN_URL and ECHO_SERVER_URL environment variables set accordingly.
Happy to flesh out tests in a later PR if this is sufficient, otherwise, I may need a some guidance on where added test(s) should live for covering this method specifically.

@minauteur
Copy link
Contributor Author

OK--I've added a separate test: json_body() to assert that we get a Result::Ok back from Request::json() when we pass it a serializable type.
I also updated post_json() to use the added method for creating its requests.
An additional field: num: i16 has also been added to the structs used for both tests.
@hamza1311 let me know if you'd like me to add or amend anything--happy to help!

Thanks again for the feedback!f

Copy link
Collaborator

@ranile ranile 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. Thanks for the PR!

Just need to update the documentation and this is good to go

crates/net/src/http/mod.rs Outdated Show resolved Hide resolved
update comments per suggestion for docs

Co-authored-by: Muhammad Hamza <muhammadhamza1311@gmail.com>
@minauteur minauteur requested a review from ranile April 4, 2022 17:19
@minauteur
Copy link
Contributor Author

✔️ Thanks so much!

@ranile
Copy link
Collaborator

ranile commented Apr 5, 2022

Thanks for the PR. This is good to go

@minauteur
Copy link
Contributor Author

Awesome--feel free to merge whenever!
Not sure if the comment needs that line deleted from the linting check--happy to do that if so

@ranile
Copy link
Collaborator

ranile commented Apr 6, 2022

Please ensure that the CI is green

@minauteur
Copy link
Contributor Author

minauteur commented Apr 6, 2022

Please ensure that the CI is green

Looks like you (or another maintainer) will need to approve running the workflow, but it should pass with the latest commit!
Awesome, everything looks good! Thanks again!
🎉

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.

2 participants