Electric-Coin-Company / zcash-swift-wallet-sdk

iOS light client Framework proof-of-concept
MIT License
37 stars 33 forks source link

Allow custom logger or provide way to disable logging altogether #1013

Closed tw0po1nt closed 1 year ago

tw0po1nt commented 1 year ago

Is your feature request related to a problem? Please describe.

Currently, the SDK logs internal events using its own, internal logger. The documentation describes the ability to customize this logger by passing an object conforming to the Logger protocol to the Initialization object. (see readme However, the SDK currently only provides a convenience init() that allows users of the SDK to pass in a log level, not a custom instance of Logger. Aside from this being confusing, it also means that the SDK doesn't actually allow customization of the logger beyond the log level.

Describe the solution you'd like

My use case is to disable logging altogether. To that end:

I'd like for Initializer struct to provide an additional init function such as the following:

convenience public init(
        cacheDbURL: URL?,
        fsBlockDbRoot: URL,
        dataDbURL: URL,
        pendingDbURL: URL,
        endpoint: LightWalletEndpoint,
        network: ZcashNetwork,
        spendParamsURL: URL,
        outputParamsURL: URL,
        saplingParamsSourceURL: SaplingParamsSourceURL,
        alias: ZcashSynchronizerAlias = .default,
        logger: Logger? = nil
) { ... }

For a user who doesn't care about customizing the logger, they can omit the logger parameter, in which case the SDK will default to its own, internal logger that it is using today.

For a user who wants to provide custom functionality to their logger, they can implement the protocol and pass in the conforming object. The SDK will use that to perform all logging.

For a user (such as myself) who wants to disable logging entirely, I can implement the Logger protocol and leave all the methods empty.

Alternatives you've considered

Alternatively, since I simply would like the ability to disable logging, one approach to that could be to have an initializer that accepts a flag to determine whether logging is enabled or disabled. If this alternative is chosen as the preferred approach, I think we should update the README to remove any mention of passing in a custom logger.

pacu commented 1 year ago

Thanks for pointing this out!.

The main disadvantage of an initializer that accepts an optional logger is that it doesn't describe what nullability means. Also it makes every client have to create a logger of their own.

maybe we could thing of a meaningful value that provides intent and leaves no room for interpretation.

public enum LoggingPolicy {
    case `default`(LogLevel)
    case custom(Logger)
    case noLogging

Thoughts?

cc'ing @Chlup @LukasKorba

tw0po1nt commented 1 year ago

I think that approach sounds fine, but I'm also interested to see what others think.

@pacu Once decided, would you like me to open a PR with these changes?

pacu commented 1 year ago

yes that would be great!