-
Notifications
You must be signed in to change notification settings - Fork 316
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
custom logger cleanup #515
Conversation
fa76a6e
to
5983a65
Compare
@@ -28,9 +22,9 @@ NS_SWIFT_NAME(Purchases.Logger) | |||
+ (void)setLogHandler:(void(^)(RCLogLevel, NSString * _Nonnull))logHandler; | |||
|
|||
/** | |||
Enable debug logging. Useful for debugging issues with the lovely team @RevenueCat | |||
Used to set the log level. Useful for debugging issues with the lovely team @RevenueCat |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<3
+ (void)setLogLevel:(RCLogLevel)logLevel { | ||
switch (logLevel) { | ||
case RCLogLevelDebug: | ||
RCLog.logLevel = RCInternalLogLevelDebug; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
curious about the need for a separate internal log level?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the problem to solve here is that logLevel is defined in the swift module, but used and exposed publicly by the objc module.
So the idea here is to have an internal one used for swift, and replicate its structure for the public one in objc.
An alternative approach is to define the exact same interface in a header file in objective c as what we have in swift. This would allow us to have a single enum.
This is what we do for Transaction.swift (objc here, swift here), but I think there were some issues doing this for enums, where you'd get errors when archiving because they'd be identified as duplicate symbols.
I'm going to take another look to see if matching the exact declarations would work, but going with the duplicate internal
enum is safer in that we won't face the duplicate symbols issue when archiving
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahh ok i think i get it. one thought i had would be setting the private enum value as a val on the public enum, then at least you can use property access to avoid the switches?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you elaborate? do you mean just setting them to the same Int value?
|
||
// These must match the values in LogLevel.swift exactly | ||
typedef NS_ENUM(NSInteger, RCLogLevel) { | ||
RCLogLevelDebug = 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i meant here, rather than setting the value 0,1,2,3 (or in addition to, if you use those values), you could have the internal log level. just creates an easier mapping
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh nice! that's much smarter than what I was doing, I'll update. thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, right, just remembered why we can't do that 😢
You can't forward-declare enum values in objective-c, so there's no way to reference the values from this header file, sadly.
…e purchases object directly
…y purpose is to bridge to Swift
7a31a8f
to
fc8efe1
Compare
This is a follow-up to #481, which lets developers set up a custom hook to handle logs.
This PR cleans up the public methods a bit, granting developers direct access to the log level they want.
Changes summary:
logLevel
andsetLogHandler
directly available fromPurchases
, without having to go through theRCLogger
object.logLevel
enum into its own public headerLogging
group for related filesRCLogger
->RCLoggerBridge
, since its sole purpose is to bridge stuff to the Swift logger, not really to log.shouldShowDebugLogs
stuff and using the new log levels instead.