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

Fixed NetworkLoggerPlugin behavior with errors. #1880

Merged
merged 10 commits into from Aug 5, 2019

Conversation

@amaurydavid
Copy link

commented Jul 8, 2019

Hi!

So the last few days I've been dealing with an API that errors a lot, and I was frustrated to se that when using the NetworkLoggerPlugin, errors would not be correctly displayed.

If using the Alamofire http codes validation and the API responds with a HTTP 500, NetworkLoggerPlugin would just log Received empty network response for ..., without any details put in the response's body by the API. To get them I had to copy the cURL and re-run manually the request in to get the full response detail.

So I dived in NetworkLoggerPlugin and made a few changes.
First, I now fetch the error's response in the didReceive(:target) if there is any.
Second, I added a "Body: " prefix before the body content so that we can retrieve it more easily from the logs.

Examples using the same request:
Before:
["Moya_Logger: [08/07/2019 16:28:54] Response: Received empty network response for <Here is a description of the target>"]
After:
["Moya_Logger: [08/07/2019 15:56:08] Response: <NSHTTPURLResponse: 0x1c002eaa0> { URL: <Some URL Here> } { Status Code: 500, Headers { <Some headers here>} }", "Body: {\"error\":{\"message\":\"Here is an error message from the API\",\"code\":178912}}"]

edit: I will update tests so that they don't brake.

@amaurydavid amaurydavid changed the title Fixed NetworkLoggerPlugin behavior with errors. [WIP] Fixed NetworkLoggerPlugin behavior with errors. Jul 9, 2019

@MoyaBot

This comment has been minimized.

Copy link

commented Jul 10, 2019

1 Warning
⚠️ Consider adding supporting documentation to this change. Documentation can be found in the docs directory.
3 Messages
📖 iOS: Executed 279 tests, with 0 failures (0 unexpected) in 13.428 (13.614) seconds
📖 tvOS: Executed 279 tests, with 0 failures (0 unexpected) in 13.148 (13.322) seconds
📖 macOS: Executed 279 tests, with 0 failures (0 unexpected) in 13.244 (13.375) seconds

Generated by 🚫 Danger

@codecov-io

This comment has been minimized.

Copy link

commented Jul 10, 2019

Codecov Report

Merging #1880 into development will decrease coverage by 0.17%.
The diff coverage is 95.45%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #1880      +/-   ##
===============================================
- Coverage        92.29%   92.12%   -0.18%     
===============================================
  Files               26       25       -1     
  Lines              909      876      -33     
===============================================
- Hits               839      807      -32     
+ Misses              70       69       -1
Impacted Files Coverage Δ
Sources/Moya/Plugins/NetworkLoggerPlugin.swift 93.05% <95.45%> (-1.07%) ⬇️
Sources/Moya/Moya+Alamofire.swift 82.6% <0%> (-2.4%) ⬇️
Sources/Moya/MoyaProvider+Internal.swift 97.01% <0%> (-1.56%) ⬇️
Sources/Moya/MoyaProvider.swift 91.66% <0%> (ø) ⬆️
Sources/Moya/RequestTypeWrapper.swift
Sources/Moya/MoyaProvider+Defaults.swift 80% <0%> (+1.73%) ⬆️

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 480d095...3962b93. Read the comment docs.

amaurydavid added 3 commits Jul 15, 2019

@amaurydavid amaurydavid changed the title [WIP] Fixed NetworkLoggerPlugin behavior with errors. Fixed NetworkLoggerPlugin behavior with errors. Jul 16, 2019

@amaurydavid

This comment has been minimized.

Copy link
Author

commented Jul 16, 2019

I've reworked a bit the logging to improve once again the behavior, and also updated tests.
The PR is now ready to be reviewed :)

@sunshinejr

This comment has been minimized.

Copy link
Member

commented Jul 27, 2019

Hey @amaurydavid, sorry for the delay, didn't see that you finish the work earlier! Really nice improvement, got one question but other than that it looks great. I think many people will appreciate the updates to the NetworkLoggerPlugin, myself included 😉

@sunshinejr
Copy link
Member

left a comment

Hey @amaurydavid, would you be able to also add a changelog entry and check if we need to update our existing documentation for this plugin? Thanks!

@sunshinejr sunshinejr referenced this pull request Aug 1, 2019
amaurydavid added 2 commits Aug 4, 2019
@amaurydavid

This comment has been minimized.

Copy link
Author

commented Aug 4, 2019

Hey @amaurydavid, would you be able to also add a changelog entry and check if we need to update our existing documentation for this plugin? Thanks!

I've added a changelog entry, the actual doc seems fine to me.

@sunshinejr

This comment has been minimized.

Copy link
Member

commented Aug 4, 2019

Awesome @amaurydavid! Do you think you can also resolve the conflicts? Sorry, we released new alpha version in the meantime...

@MoyaBot

This comment has been minimized.

Copy link

commented Aug 5, 2019

Warnings
⚠️

Any changes to library code should be reflected in the Changelog.
Please consider adding a note there and adhere to the Changelog Guidelines.

Messages
📖 iOS: Executed 278 tests, with 0 failures (0 unexpected) in 13.464 (13.630) seconds
📖 tvOS: Executed 278 tests, with 0 failures (0 unexpected) in 13.023 (13.193) seconds
📖 macOS: Executed 278 tests, with 0 failures (0 unexpected) in 14.469 (14.606) seconds

Generated by 🚫 Danger Swift against 3962b93

@sunshinejr
Copy link
Member

left a comment

Thanks a lot @amaurydavid! Gonna ship it in the next release 🚀

@sunshinejr sunshinejr merged commit a062ef7 into Moya:development Aug 5, 2019

0 of 2 checks passed

ci/circleci: build CircleCI is running your tests
Details
ci/circleci: carthage_without_swiftlint_integration Your tests are queued behind your running builds
Details
@peril-moya

This comment has been minimized.

Copy link

commented Aug 5, 2019

@amaurydavid Thanks a lot for contributing to Moya! We've invited you to join
the Moya GitHub organization – no pressure to accept! If you'd like more
information on what that means, check out our contributor guidelines and
feel free to reach out to @Moya/core-team with any questions.

Generated by 🚫 dangerJS

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.