Added ability to log errors #33
Conversation
Added ability to log errors when invalid values are found in the value providers, and wrote tests
albertodebortoli
left a comment
albertodebortoli
left a comment
There was a problem hiding this comment.
I have bad memories of using closures like that and in the end it regretting it.
errorLogger is retained within ToggleManager which could cause retain cycles if the caller does nasty things (for example, the closure retains another object passed in the ToggleManager's init).
I think it’s best if we use a protocol.
The name “errorLogger” implies it's used only in case of errors, but the signature doesn’t. With a protocol it’d be more explicit and the parameter should be best renamed to just “logger” (with the protocol being Logging).
Not too sure about toggleTypeDescription and removing the semi equatable method. It's only used in the tests, so maybe you can add the property in an extension in the test target?
Updated to use Protocol and Custom Error type
| /// Thread-safe facade to interface with toggles. | ||
| final public class ToggleManager: ObservableObject { | ||
|
|
||
| public enum ProviderValueError: Error { |
There was a problem hiding this comment.
This is best placed in Logging.swift before the protocol definition.
Also best to not use the word provider at this level, I would suggest ToggleError.
Sources/Protocols/ErrorLogger.swift
Outdated
|
|
||
| import Foundation | ||
|
|
||
| public protocol ErrorLogger { |
There was a problem hiding this comment.
Can you rename to Logger so it's easier to extend in the future? Header, filename and vars included.
Sources/Protocols/ErrorLogger.swift
Outdated
| import Foundation | ||
|
|
||
| public protocol ErrorLogger { | ||
| func logError(_ error: ToggleManager.ProviderValueError) |
There was a problem hiding this comment.
The signature of the method can be without “Error” in the name as the type of the parameter gives it away:
func log(_ error: ToggleError)
| if shouldCheckInvalidValueTypes, let defaultValue, !(value ~= defaultValue) { | ||
| private func isValueValid(value: Value, defaultValue: Value?, variableName: Variable, providerName: String) -> Bool { | ||
| if shouldCheckInvalidValueTypes, let defaultValue, value.toggleTypeDescription != defaultValue.toggleTypeDescription { | ||
| errorLogger?.logError(ProviderValueError.invalidValueType("\(variableName) was found with an invalid type (\(value.toggleTypeDescription)) in Provider: \(providerName)")) |
There was a problem hiding this comment.
It would be better to avoid hard-coded English strings in code. All we need is to pass the error type for which we have an enum already. That's what brings the information.
logger?.log(.invalidValueType(variable, value, provider))
There was a problem hiding this comment.
this would remove the need for toggleTypeDescription
| let defaultValue: Value? = defaultValueProvider.optionalValue(for: variable) | ||
|
|
||
| if let value = mutableValueProvider?.value(for: variable), isValueValid(value: value, defaultValue: defaultValue) { | ||
| if let value = mutableValueProvider?.value(for: variable), isValueValid(value: value, defaultValue: defaultValue, variableName: variable, providerName: mutableValueProvider?.name ?? "MutableValueProvider") { |
There was a problem hiding this comment.
hmm, not sure I like this hard-coded string nor the fallback
| } | ||
|
|
||
| if shouldCheckInvalidSecureValues, ((try? readValue(for: value)) == nil) { | ||
| errorLogger?.logError(ProviderValueError.insecureValue("\(variableName) was found with an invalid secure value in Provider: \(providerName)")) |
There was a problem hiding this comment.
as above
logger?.log(. insecureValue(variable, provider))
| final public class ToggleManager: ObservableObject { | ||
|
|
||
| public enum ProviderValueError: Error { | ||
| case invalidValueType(String) |
There was a problem hiding this comment.
can we be explicit?
case invalidValueType(Variable, Value, ValueProvider)
|
|
||
| public enum ProviderValueError: Error { | ||
| case invalidValueType(String) | ||
| case insecureValue(String) |
There was a problem hiding this comment.
same here
case insecureValue(Variable, ValueProvider)
| if let value = mutableValueProvider?.value(for: variable), isValueValid(value: value, defaultValue: defaultValue) { | ||
| return value | ||
| if let value = mutableValueProvider?.value(for: variable) { | ||
| if isValueValid(value: value, defaultValue: defaultValue, variableName: variable, provider: mutableValueProvider!) { |
There was a problem hiding this comment.
There's a force unwrap here, best to use if let.
if let mutableValueProvider, value = mutableValueProvider.value(for: variable) {
if isValueValid(value: value, defaultValue: defaultValue, variableName: variable, provider: mutableValueProvider) {
2 participants
Added the ability to log errors when invalid value types / invalid secure values are found in the value providers,.
Also wrote a bunch of tests.
Reinstated
toggleTypeDescriptionas it came in useful here as well and removed the semi equatable definition. It used in the logs for describing the Toggle type. (in response to comment, it is used in ToggleManager in addition to tests)Following feedback I have:
Replaced the closure with an
ErrorLoggerprotocolI have kept the name as errorLogger but changed the signature to only accept
Errortypes.