Preventing logging if ownershipType key is missing #1399
Conversation
| func decode<T: Decodable>(_ type: T.Type, | ||
| forKey key: Self.Key, | ||
| defaultValue: T, | ||
| logDecodingError: Bool = true) -> T { |
There was a problem hiding this comment.
I mentioned it in the description, but maybe we want to not log anytime there's a defaultValue instead of adding a new parameter to this function.
There was a problem hiding this comment.
maybe we want to not log anytime there's a defaultValue instead of adding a new parameter to this function.
That makes sense to me. Maybe make it debug only?
There was a problem hiding this comment.
It looks like the issue is about this polluting debug logs so that wouldn't really fix this issue, would it? 🤔 Do we have a verbose setting we could log this under?
There was a problem hiding this comment.
We do have a verbose setting, but it's currently used for controlling whether the file name, method name and line number are also logged.
I don't think there's a lot of value in logging whenever an optional key isn't found. If it's considered optional, then it doesn't seem like an event worth notifying.
So I'd remove the log. I believe we put it in place when debugging an issue with a customer whose PurchaserInfo object couldn't be parsed (it turned out to be a localization issue with our ISO date formatter).
I think we even log the full received CustomerInfo somewhere so this log would be redundant anyway?
There was a problem hiding this comment.
That said, I would expect debug logs to be noisy at least to a certain extent, since they're supposed to be used exclusively for debugging
| @@ -83,7 +83,6 @@ extension KeyedDecodingContainer { | |||
| do { | |||
| return try decode(type, forKey: key) | |||
| } catch { | |||
| ErrorUtils.logDecodingError(error) | |||
There was a problem hiding this comment.
Yes it's used in JSONDecoder.decode. But that one throws an error instead of using a default value so I think it makes sense to keep it there
|
Thank you for taking care of this. I added this code to help with debugging customer issues around decoding and thought it might be useful to keep around. This created way more noise than intended, super sorry about that @EliotAndres |
Fixes #1392
I am not sure about the fix, so open to suggestions. For now I made the logging optional, but maybe we want to not log anytime there's a
defaultValue.CF-366