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

Adding more customization to NetworkLoggerPlugin. #1894

Merged
merged 13 commits into from Sep 2, 2019

Conversation

@amaurydavid
Copy link

commented Aug 5, 2019

Following the discussion with @sunshinejr in #1880, here is a draft of a potential rework of the NetworkLoggerPlugin.

The issue we had was that response's body was only logged if we were in verbose mode. This means that if a request returns with an error, the error's body wasn't logged if not in verbose. It's ok if you don't want it, but it can be necessary according to your workflow. So we had to provide an option so that responses' bodies are logged in errors.
I could have used a simple enum LogLevel { case error(showBodies: Bool), info, verbose } but I wanted the user to fully customize the logs.

So I came with the following idea: list all the components that can be logged in requests & responses, and allow the user to opt-in for each one.
That's the purpose of the new RequestLoggingOptions and ResponseLoggingOptions.

And to handle the issue we were facing with bodies in errors, there are 2 sets of options for responses: the successResponseLoggingOptions will be used when the response returns as a success, and errorResponseLoggingOptions when it's an error. This way, we can configure the logger to log the error's body, but not success's.

Additionnaly, I've moved all of the plugin's config into a.... Configuration struct, making the Plugin more readable imo.

So it's just a draft of the direction I want to go, It's not even tested and things may be broken atm.
But I want to have some feedback before spending some time on it: do you feel it may be a suitable rework for NetworkLoggerPlugin?

amaurydavid added 5 commits Aug 6, 2019

@amaurydavid amaurydavid changed the title [WIP] Adding more customization to NetworkLoggerPlugin. Adding more customization to NetworkLoggerPlugin. Aug 15, 2019

@amaurydavid amaurydavid marked this pull request as ready for review Aug 15, 2019

fileprivate let cURLTerminator = "\\\n"
fileprivate let output: (_ separator: String, _ terminator: String, _ target: TargetType, _ items: Any...) -> Void
fileprivate let requestDataFormatter: ((Data) -> (String))?
fileprivate let responseDataFormatter: ((Data) -> (Data))?

This comment has been minimized.

Copy link
@amaurydavid

amaurydavid Aug 15, 2019

Author

Not sure here why both dataFormatters did not have the same return type. As for now, i've made both of them return a String - which I believe is the logical choice for a logger -, but if it causes some trouble let me now.

@@ -250,8 +251,7 @@ final class MoyaProviderIntegrationTests: QuickSpec {
provider.request(.zen) { _ in done() }
}

expect(log).to(contain("Request:"))
expect(log).to(contain("{ URL: https://api.github.com/zen }"))
expect(log).to(contain("Request: https://api.github.com/zen"))

This comment has been minimized.

Copy link
@amaurydavid

amaurydavid Aug 15, 2019

Author

The request's description wasn't correct here. I don't think it's due to my work but rather an update of AF 5 that wasn't fixed.


expect(log).to(contain("Response:"))
expect(log).to(contain("{ URL: https://api.github.com/zen }"))
expect(log).to(contain("formatted request body"))

This comment has been minimized.

Copy link
@amaurydavid

amaurydavid Aug 15, 2019

Author

There seemed to be a mismatch between the test's name and its content: it was supposed to check the requestDataFormatter, but acted like it was testing the response. So I fixed it by trully checking the request.

@amaurydavid

This comment has been minimized.

Copy link
Author

commented Aug 15, 2019

I didn't have any feedback on the draft so I continued along the path.
Tests are now ok.
There are some changes I've made to simplify the previous behavior, but if it's a mistake let me now.
I've added some comments in the review on shadowy points.

@MoyaBot

This comment has been minimized.

Copy link

commented Aug 15, 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.

⚠️

Consider also updating the English docs. For Chinese translations, request the modified file(s) to be added to the list here for someone else to translate, if you can't do so yourself.

⚠️

Consider adding supporting documentation to this change. Documentation can be found in the docs directory.

Messages
📖 iOS: Executed 278 tests, with 0 failures (0 unexpected) in 13.259 (13.412) seconds
📖 tvOS: Executed 278 tests, with 0 failures (0 unexpected) in 13.209 (13.362) seconds
📖 macOS: Executed 278 tests, with 0 failures (0 unexpected) in 14.359 (14.494) seconds

Proselint report on docs/Plugins.md

Line Message Severity
29 Substitute 'damn' every time you're inclined to write 'very'; your editor will delete it and the writing will be just as it should be. Found once elsewhere. warning

Mdspell report on docs/Plugins.md:

Line Typo
6 e.g.
9 e.g.
29 iOS

Mdspell report on docs/MigrationGuides/migration_13_to_14.md:

Line Typo
1 13.x
1 14.x
3 Versioning
5 NetworkLoggerPlugin
6 instanciate

Generated by 🚫 Danger Swift against 12d7515

@codecov-io

This comment has been minimized.

Copy link

commented Aug 15, 2019

Codecov Report

Merging #1894 into development will increase coverage by 0.21%.
The diff coverage is 94.11%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #1894      +/-   ##
===============================================
+ Coverage        92.27%   92.48%   +0.21%     
===============================================
  Files               26       26              
  Lines              919      932      +13     
===============================================
+ Hits               848      862      +14     
+ Misses              71       70       -1
Impacted Files Coverage Δ
Sources/Moya/Plugins/NetworkLoggerPlugin.swift 95.6% <94.11%> (+2.01%) ⬆️
Sources/Moya/RequestTypeWrapper.swift 76.92% <0%> (ø) ⬆️

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 a145cf3...12d7515. Read the comment docs.

public extension NetworkLoggerPlugin {
struct Configuration {

public typealias OutputType = (_ target: TargetType, _ items: [Any]) -> Void

This comment has been minimized.

Copy link
@MoyaBot

MoyaBot Aug 16, 2019

  • ⚠️ Types should be nested at most 1 level deep (nesting)
struct Configuration {

public typealias OutputType = (_ target: TargetType, _ items: [Any]) -> Void
public typealias DataFormatterType = (Data) -> (String)

This comment has been minimized.

Copy link
@MoyaBot

MoyaBot Aug 16, 2019

  • ⚠️ Types should be nested at most 1 level deep (nesting)

This comment has been minimized.

Copy link
@LucianoPAlmeida

LucianoPAlmeida Aug 23, 2019

Member

Should we set the swiftlint flag to ignore this? @amaurydavid @sunshinejr

This comment has been minimized.

Copy link
@amaurydavid

amaurydavid Aug 23, 2019

Author

Definitively, though I'm not sure whether to disable it for the whole project or just those two typealiases.

This comment has been minimized.

Copy link
@LucianoPAlmeida

LucianoPAlmeida Aug 23, 2019

Member

I think we could just let for those two. Maybe this warning is good to have and decide if we fix or disable the warning on a case by case basis. cc @sunshinejr thoughts?

@sunshinejr
Copy link
Member

left a comment

Hey @amaurydavid, thanks for doing this! And sorry for the late review. This is a really big change to the plugin and I wanted to take my time and think in what direction I would like to go with that.

And this is a very good start! I really like that we extracted all the options to a separate entity and that we are able to customize a lot more things. Additionally, as our Vision.md states:

provide an API where common things are easy, and uncommon things are possible.

I think we are doing a very good job on the flexibility of the API, but we need to improve on the common use cases. E.g. when I would like to print everything, now instead of just using verbose: true I would need to do:

let provider = MoyaProvider<MultiTarget>(plugins: [NetworkLoggerPlugin(configuration: .init(requestLoggingOptions: .verbose,
	                                                                                            successResponseLoggingOptions: .verbose,
	                                                                                            errorResponseLoggingOptions: .verbose))])

which, I think we can agree, is not ideal. I know that this allows us for a lot better and more flexible logging options, but the popular use case suffers. Maybe we could try to use enum for the logging option and include what we had before + a custom one? E.g.

enum LoggingOption {
    default
    verbose
    custom(request: RequestLogginOptions, response: ResponseLoggingOptions...etc.)
}

But I think in that case we would need to move cURL option somewhere else as it won't really fit in there (all of them are available printing parts but cURL is a response-output formatter rather.

Let me know what do you think about it!

@amaurydavid

This comment has been minimized.

Copy link
Author

commented Aug 20, 2019

I agree, the necessity of having to use .verbose on all 3 parameters struck me when updating the tests.
However i think the cURL option should stay with other options because the cURL format rules which component is logged and which is not.
Another solution could also be to reunite RequestLoggingOptions and ResponseLoggingOptions as ResponseLoggingOptions only has 1 useful option.
We would only have to rename RequestLoggingOptions into LoggingOptions, and add a successResponseBody and errorResponseBody option.

@sunshinejr

This comment has been minimized.

Copy link
Member

commented Aug 22, 2019

@amaurydavid thanks, I think it looks better! gonna checkout the branch and play with the logger :)

cc @LucianoPAlmeida @pedrovereza are u guys up for a CR maybe?

amaurydavid added 2 commits Aug 22, 2019
@@ -231,16 +231,11 @@ final class MoyaProviderIntegrationTests: QuickSpec {

describe("a provider with network logger plugin") {
var log = ""
var plugin: NetworkLoggerPlugin!
let plugin = NetworkLoggerPlugin(configuration: .init(output: { log += $1.joined() },

This comment has been minimized.

Copy link
@sunshinejr

sunshinejr Aug 23, 2019

Member

oh nice this looks really clean now!

fileprivate let dateFormatter = DateFormatter()
fileprivate let separator = ", "
fileprivate let terminator = "\n"
fileprivate let cURLTerminator = "\\\n"

This comment has been minimized.

Copy link
@LucianoPAlmeida

LucianoPAlmeida Aug 23, 2019

Member

Did we remove those constants and are using that hard coded now or they are not needed anymore?

This comment has been minimized.

Copy link
@amaurydavid

amaurydavid Aug 23, 2019

Author

Previously the dateFormatter was set using a dateFormat provided in the NetworkLoogerPlugin's init. Now we have to directly provide the DateFormatter, which allow use to set it up using an hard coded dateFormat, or Apple's dateStyle and timeStyle. The default value uses a short date and time style.

separator and terminator are now hardCoded in the default output method.
cURLTerminator is no longer used.

This comment has been minimized.

Copy link
@LucianoPAlmeida
@LucianoPAlmeida
Copy link
Member

left a comment

This looks nice 👍
Aside for a minor question, LGTM @sunshinejr @amaurydavid

amaurydavid added 2 commits Aug 23, 2019
@@ -0,0 +1,9 @@
# Migration Guide from 13.x to 14.x

This comment has been minimized.

Copy link
@sunshinejr
@sunshinejr
Copy link
Member

left a comment

@amaurydavid this looks awesome. Thank you for your time and patience on this one. I love the outcome and I can't wait to use it in the next Moya version 🚀

@sunshinejr sunshinejr merged commit a93d53f into Moya:development Sep 2, 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.