Expose logger protocol #10

wtrocki · 2018-01-31T17:00:31Z

Motivation

Expose logger protocol and hide actual implementation.

2f7f884 contains actual changes

josemigallas · 2018-02-02T08:51:26Z

modules/core/AgsCore.swift

-    public var logger: XCGLogger { return AgsCoreLogger.logger() }
+    public static let logger: AgsLoggable = {
+        let log = XCGLogger(identifier: "AeroGearSDK", includeDefaultDestinations: true)
+        log.setup(level: .debug, showThreadName: false, showLevel: true, showFileNames: true, showLineNumbers: true)


It is a bit confusing that here .debug level is being chosen but then you must specify the level again when calling an AgsLoggable.

I believe this is for the default configuration only but the interface doesn't allow to do any log without specifying a level:

AgsCore.logger.error(...) AgsCore.logger.debug(...)

We can't do something like AgsCore.logger.log(...) can we? For that reason I think that all default or initial values should be moved to XCGLoggerAdapter.

Logger is used across the code by picking one of the methods like AgsCore.logger.error etc.
Depending on logger level above this statements will be executed or not.

AgsCore.logger.log(...) can we?

I think this way it will look more ugly (and users needs to use keys from new enum)
AgsCore.logger.log(level: .asgLevelDebug, msg)

Your comment inspired me to not hardcode this level and allow people to control that. Going to add option to change level outside library and document that!

josemigallas · 2018-02-02T08:52:58Z

Small comment, otherwise LGTM!

troZee · 2018-02-02T08:56:43Z

example/AeroGearSdkExample/ViewController.swift

        let jsonEncoder = JSONEncoder()
        do {
            let jsonData = try jsonEncoder.encode(currentConfig)
            let jsonString = String(data: jsonData, encoding: .utf8)
-            print("JSON String : " + jsonString!)
+            AgsCore.logger.debug("JSON String \(jsonString!)")


Use ?? instead of !. Try to avoid !.

I'm just wondering, can be here ever jsonString null? My Swift knowledge is still very basic.

@wojta jsonString is optional so it can be nil (null). Is safer to use ??, because if you user force unwrap ! and jsonString will be nil(null) the app will crash. ?? means that you want to provide default value in case of nil. In this case it will be jsonString ?? ""

Some init methods for String produces optionals:
https://developer.apple.com/documentation/swift/string/1418413-init

Fixing that now.

@wtrocki I really confused it with Kotlin where you don't have this construct, String() always returns non-nullable type.

danielpassos · 2018-02-02T17:58:17Z

modules/core/logger/AgsLoggable.swift

+* Class extends AgsLoggable protocol without providing implementation
+* Can be used to disable logging
+*/
+open class AgsDisabledLogger: AgsLoggable {


It's something really personal but I don't like to have more than one class/stuff in the same file. I'd like to see this in a separated file.

Swift has no concept of single class per file but I think thats good pattern to stick with for this SDK.

Like I said, this is not about right or wrong stuff, it's a personal opinion. I think 1 file per class makes the project a little more organized and easy to understand, especially for people not on the project to see the file and identify what is this about.

More than one class per file sometimes hides things we have ;)

danielpassos · 2018-02-02T18:01:41Z

modules/core/AgsCore.swift

+    /**
+    * Logger instance used for logging across SDK's
+    */
+    public static var logger: AgsLoggable = {
        let log = XCGLogger(identifier: "AeroGearSDK", includeDefaultDestinations: true)
        log.setup(level: .debug, showThreadName: false, showLevel: true, showFileNames: true, showLineNumbers: true)


I think showFileNames is making thing more confused than helping. I mean...

Running the example I got:

2018-02-02 15:29:23.751 [Info] > AeroGearSdkExample Version: 1.0 Build: 1 PID: 78974 2018-02-02 15:29:23.751 [Info] > XCGLogger Version: 6.0.1 - Level: Debug 2018-02-02 15:29:23.764 [Debug] [XCGLoggerAdapter.swift:34] debug > Initializing AeroGearServices Core SDK 2018-02-02 15:29:23.764 [Debug] [XCGLoggerAdapter.swift:34] debug > Parsing configuration mobile-services 2018-02-02 15:29:32.096 [Debug] [XCGLoggerAdapter.swift:34] debug > Loading configuration 2018-02-02 15:29:32.099 [Debug] [XCGLoggerAdapter.swift:34] debug > JSON String {"name":"sync","config":{"uri":"https:\/\/www.mocky.io\/v2\/5a6b59fb31000088191b8ac6"}} 2018-02-02 15:29:33.173 [Debug] [XCGLoggerAdapter.swift:34] debug > Loading configuration 2018-02-02 15:29:33.174 [Debug] [XCGLoggerAdapter.swift:34] debug > JSON String {"name":"prometheus","config":{"uri":"https:\/\/www.mocky.io\/v2\/5a6b59fb31000088191b8ac6"}}

Initializing AeroGearServices Core SDK for example is a log from AgsCore not from XCGLoggerAdapter.

Do you got my point here?

That is a bug related with using wrapper without passing actual directives. I will need to replicate this API. Really good work. I totally missed that

wtrocki · 2018-02-05T12:33:31Z

@danielpassos Addressed both comments

danielpassos · 2018-02-05T13:06:55Z

modules/core/logger/XCGLoggerAdapter.swift

+     * @param closure  A closure that returns the object to be logged. It can be any object like string, array etc.
+     * @param userInfo Dictionary for adding arbitrary data to the log message, can be used by filters/formatters etc
+     */
+    open override func verbose(functionName: StaticString = #function, fileName: StaticString = #file, lineNumber: Int = #line, _ closure: @autoclosure () -> Any?) {


I think we should use all of this XCGLogger kinds of stuff, but IMO we should not provide options for it. Maybe other libs don't provide it. I wanna make it simple only receiving a message. wdyt?

This implementation is provided by IOS platform compiler and bind on build.
User api require only message. I investigated couple options and most of the libraries using that.
This is a cost of using wrapper and underlying library can use or totally ignore this additional fields.

Users will never need to provide them (they can if they will, but this fields are bind on complication) I could add more documentation to this implementation to explain that.

danielpassos · 2018-02-05T13:07:34Z

modules/core/AgsCore.swift

-    public var logger: XCGLogger { return AgsCoreLogger.logger() }
+    public static var logger: AgsLoggable = {
+        let log = XCGLogger(identifier: "AeroGearSDK", includeDefaultDestinations: true)
+        log.setup(level: .debug, showThreadName: false, showLevel: true, showFileNames: true, showLineNumbers: true)


This kind of config still needed here?

This is standard config that will evolve over the time depending on what SDK will log etc.
Current configuration provides sensible defaults that are needed. We need to setup log level somewhere etc.

Yeap, my point is if we should not move it into the adapter implementation, but it's not required/important/blocker

wtrocki requested review from danielpassos, aidenkeating, josemigallas and compjuicer January 31, 2018 17:00

wtrocki added 3 commits February 1, 2018 16:07

Minor improvements to demo app

710a8a2

Format log messages

f03a03d

Main logger interface implementation

48f0a09

wtrocki force-pushed the wrapper-logger branch from 2f7f884 to 48f0a09 Compare February 1, 2018 16:08

josemigallas reviewed Feb 2, 2018

View reviewed changes

troZee reviewed Feb 2, 2018

View reviewed changes

Added option to swap logger

6953854

wtrocki force-pushed the wrapper-logger branch from e66f657 to 6953854 Compare February 2, 2018 10:52

danielpassos suggested changes Feb 2, 2018

View reviewed changes

Change interface to provide line numbers and filenames

acd8317

wtrocki requested a review from pb82 February 5, 2018 12:34

danielpassos suggested changes Feb 5, 2018

View reviewed changes

danielpassos approved these changes Feb 5, 2018

View reviewed changes

danielpassos merged commit 06b6c86 into aerogear:master Feb 5, 2018

wtrocki deleted the wrapper-logger branch February 5, 2018 13:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose logger protocol #10

Expose logger protocol #10

wtrocki commented Jan 31, 2018

josemigallas Feb 2, 2018

wtrocki Feb 2, 2018

josemigallas commented Feb 2, 2018

troZee Feb 2, 2018

wojta Feb 2, 2018

troZee Feb 2, 2018 •

edited

wtrocki Feb 2, 2018

wojta Feb 2, 2018

danielpassos Feb 2, 2018

wtrocki Feb 2, 2018 •

edited

danielpassos Feb 2, 2018 •

edited

wtrocki Feb 2, 2018

danielpassos Feb 2, 2018 •

edited

wtrocki Feb 2, 2018

wtrocki commented Feb 5, 2018

danielpassos Feb 5, 2018

wtrocki Feb 5, 2018 •

edited

danielpassos Feb 5, 2018

wtrocki Feb 5, 2018

danielpassos Feb 5, 2018

Expose logger protocol #10

Expose logger protocol #10

Conversation

wtrocki commented Jan 31, 2018

Motivation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josemigallas commented Feb 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

troZee Feb 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wtrocki Feb 2, 2018 • edited

Choose a reason for hiding this comment

danielpassos Feb 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielpassos Feb 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wtrocki commented Feb 5, 2018

Choose a reason for hiding this comment

wtrocki Feb 5, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

troZee Feb 2, 2018 •

edited

wtrocki Feb 2, 2018 •

edited

danielpassos Feb 2, 2018 •

edited

danielpassos Feb 2, 2018 •

edited

wtrocki Feb 5, 2018 •

edited