Log enhancements to allow for instances #180
Conversation
| /// Subsystem of the OSLog message when running on iOS 14 or later. | ||
| open var subsystem: String { | ||
| let bundleName = Bundle.main.object(forInfoDictionaryKey: "CFBundleDisplayName") as? String | ||
| return (bundleName ?? "com.rightpoint.swiftilities") + ".log" |
There was a problem hiding this comment.
subsystem
A string that identifies the app subsystem in which to record messages. Specify this string using reverse DNS notation — for example, com.yourcompany.subsystem_name. The system uses this information to categorize and filter related log messages, and group related logging settings.
Given the definition from Apple. this seems like it should default to the bundle id of the parent application.
|
looks like CI is using Xcode 12 |
| @@ -62,7 +93,15 @@ open class Log { | |||
| /// your log statements! Use log levels appropriately | |||
| /// to keep private data out of logs that are sent over | |||
| /// the Internet. | |||
| public static var handler: ((Level, String) -> Void)? | |||
There was a problem hiding this comment.
Maybe this should be kept around and marked with a deprecated attribute, and shimmed with overridden get {} set {} to the new globalHandler
@available(*, deprecated, renamed: "globalHandler")
There was a problem hiding this comment.
it's being kept as an Instance level handler. This allow for more selective handling if there are a bunch of different Log instances floating around.
| public static var handler: InstanceLogHandler? { | ||
| get { instance.handler } | ||
| set { instance.handler = newValue } | ||
| } |
There was a problem hiding this comment.
Oh! I see the compatibility shim is down here. Might be good to mark some of these as deprecated/renamed
There was a problem hiding this comment.
do we want them deprecated? For apps where a single Log instance is sufficient, I can still see some value keeping these shorthand versions around
There was a problem hiding this comment.
Yeah perhaps not deprecated, but specifically the change related to the renaming of handler and globalHandler maybe. Also this one in particular is a bit confusing because it's an InstanceLogHandler but applied globally
There was a problem hiding this comment.
it's applied to the shared instance, but not necessarily globally. I agree the name is a little confusing though 🤔
Mainly, this is here for backwards compatibility.
Pod/Classes/Logging/Log.swift
Outdated
| public static private(set) var standard = Log("", logLevel: .off) | ||
|
|
||
| /// Static instance used for helper methods. | ||
| open class var instance: Log { | ||
| return standard | ||
| } |
There was a problem hiding this comment.
I think this is possibly still confusing for people using the old
class NetworkLog: Log {} approach, because NetworkLog.instance == Log.instance which means that NetworkLog.logLevel = .off still manipulates Log.logLevel.
I don't know if there's a good alternative approach here because there is no class var storage, so at a certain point you'll always have to reach for a static instance that is shared among all subclasses. So maybe it's best to just not even have any open override points that could potentially point to the same static instance, and just make it extra clear that setting Log.logLevel or NetworkLog.logLevel sets it for all instances and subclasses since they all refer to the same static variable.
Would also maybe suggest naming this singleton shared
There was a problem hiding this comment.
That's fair, I was attempting to still allow for a similar class NetworkLog: Log {} approach, but giving a way to override which instance gets used from the static functions. Where I ended up, is a little more verbose.
class NetworkLog: Log {
static var networkLog = Log("Network")
override class var instance: Log {
return networkLog
}
}
There was a problem hiding this comment.
Ah yeah. In that case it would be nice if there was a way to force "holding it properly", and enforce this override for subclasses, but that isn't really possible either
There was a problem hiding this comment.
I haven't found a good way to do that yet :-/
jmagnini
force-pushed
the
feature/justin/logging
branch
3 times, most recently
from
be399c7
to
14bad81
Compare
jmagnini
force-pushed
the
feature/justin/logging
branch
2 times, most recently
from
9b105c3
to
220b0ff
Compare
jmagnini
force-pushed
the
feature/justin/logging
branch
from
220b0ff
to
8a3ff8a
Compare
…rding performance data
jmagnini
force-pushed
the
feature/justin/logging
branch
from
395402e
to
4b91620
Compare
jmagnini
force-pushed
the
feature/justin/logging
branch
from
75edda5
to
832fd88
Compare
…alLogHandler if desired. Add export() to LocalLogHandler as well.
jmagnini
force-pushed
the
feature/justin/logging
branch
from
b7370e7
to
4bb2367
Compare
Allow Log to be instanced, or referenced statically. Also send log messages to OSLog when available. Additionally, Trace level messages can be used for signposts in Instruments.
See: #173