Ambiguity fix #33
Ambiguity fix #33
Conversation
# Conflicts: # Example/iOS Example/ViewController.swift # Source/Logger.swift # Tests/LogLevelTests.swift # Tests/LoggerMessageStringTests.swift # Tests/LoggerMessageTests.swift
| public var description: String { | ||
| return "\(name): \(attributes)" | ||
| } | ||
| } |
There was a problem hiding this comment.
While this is certainly clever, it is very limiting and defeats the main reason the LogMessage protocol was added in the first place. The LogMessage protocol allows you to make more robust types and to split out the information so a LogWriter can do something different with it. For example, they could send LogMessage types to NewRelic, or switch on certain attributes to report errors in a different way. There are really no limits to what you can do with the attributes as long as they are maintained all the way through to the LogWriter instances.
This change converts the name-attributes pair into a String which is then passed through Willow. Therefore, by the time the LogWriter instances get the LogMessage, it is now a String and it can't key off the attributes to do custom things.
Again, this is a clever solution that certainly removes ambiguity, but also removes a core feature of the Willow 4 release.
There was a problem hiding this comment.
As Christian says, the important part of the change was to be able to route additional (non-string) data to writers. Typically this would be context info, etc. In the case of a Console or other text writer, yes, collapsing to a single String can work.
However, when the writer wants to keep the attributes as a separate data entity that can be used for filtering, faceting, and aggregating, reducing to a string makes that impossible.
I do think there could be benefit to having Message conform to CustomStringConvertible for debugging purposes, assuming that didn't create more ambiguity with String types.
There was a problem hiding this comment.
Actually that's not what's happening.
I think you're focusing on the fact that CustomStringConvertible can necessarily produce a String, but that doesn't mean that it is a String.
Also, ll. 36-38 above is just a default implementation that can be replaced with whatever you want.
You can indeed have your cake and eat it to with this, since the LogMessage itself (and all its glory) is a CustomStringConvertible. The LogWriter can still introspect into its attributes by simply doing if let logMessage = message as? LogMessage ... see the updated test case I just pushed.
There was a problem hiding this comment.
Alright this was closed, I guess I have to open another PR.
|
Thanks again @gistya, but we're going to go ahead and close this PR out for now. If you can come up with a way to fix the ambiguity and keep the name and attributes intact, then by all means please open another PR! Cheers. 🍻 |
In reference to the problems described PR 32, it appears I may have found a way to make the compiler happy while not requiring two different method names for log and logMessage etc.
In the process I have also allowed logging any type that adopts CustomStringConvertible, so for example:
log.error {
displayNyanCat()
return someNSError.userInfo
}
I have updated Willow and its unit tests, which all pass. There are some places in the Documentation that will need info to be inserted (like what PR it was merged with). As well, the migration guide to 6.0 should be reviewed. I don't know if that's how you'd want to handle versioning, so let me know if it should be different.
Hope this works for you. If not, then let me know what could be done differently. Thanks.