-
Notifications
You must be signed in to change notification settings - Fork 18
Add support for one zero and yes no boolean strings in environement variables provider #120
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
base: main
Are you sure you want to change the base?
Conversation
|
Thank you @Adobels! A few pieces of feedback:
And just FYI, I'm going on vacation so my responses will be slower until early January. |
- Remove boolDecoder init customization - Revert changes to the default set of values in tests - Create a dedicated test to cover various string boolean variants
boolDecoderValuesForKeys -> valueForKeyOfBoolAndBoolArrayTypes
This reverts commit 5ba67cb.
|
The stringToBool test iterates over cases and highlights the input, making it easy to identify a specific string that causes a problem. Tests/ConfigurationTests/ConfigBoolsFromStringDecoderTests.swift:31 Comments have been addressed, and the PR is ready for review. |
Sources/Configuration/Providers/EnvironmentVariables/EnvironmentVariablesProvider.swift
Outdated
Show resolved
Hide resolved
Apply review comments
|
I am ready for review. |
Sources/Configuration/Providers/EnvironmentVariables/EnvironmentVariablesProvider.swift
Outdated
Show resolved
Hide resolved
Tests/ConfigurationTests/EnvironmentVariablesProviderTests.swift
Outdated
Show resolved
Hide resolved
…ntVariablesProvider.swift Use a compact switch-based implementation for decodeBool. Co-authored-by: Honza Dvorsky <honza@apple.com>
Fix the order of macros used in a test case to align with the test target order. Co-authored-by: Honza Dvorsky <honza@apple.com>
| switch string.lowercased() { | ||
| case "true", "yes", "1": true | ||
| case "false", "no", "0": false | ||
| default: nil |
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.
| default: nil | |
| default: Bool(string) |
Let’s use the built-in parser as a fallback.
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 documentation of Bool says:
If the description value is any string other than "true" or "false", the result is nil. This initializer is case-sensitive.
The modification leads to calling the Bool initializer, which is already covered by the switch and will therefore always return nil. It leads also to a question: Why Bool uses string instead of stringLowercased? This increases future maintenance
What is your reasoning behind using Bool(string)?
Another possibility is:
static func decodeBool(from string: String) -> Bool? {
let stringLowercased = string.lowercased()
return switch stringLowercased {
case "yes", "1": true
case "no", "0": false
default: Bool(stringLowercased)
}
}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.
Ah yes, stringLowercased, my point was more to still run through the initializer as a fallback.
…rings-in-EnvironementVariablesProvider
Motivation
Add support for 1, 0, and YES, NO boolean strings in EnvironmentVariablesProvider. Support for YES and NO boolean strings was not discussed in the original issue. From personal experience, I found it interesting to include it. If rejected, I will remove it. I tried to remain consitent with the existing decoders and the way they are used by the provider.
Closes #110
Modifications
The EnvironmentVariablesProvider's bool decoder is configurable and can be initialized with support for true/false, 1/0, yes/no, or any combination of them.
Result
Usage of EnvironmentVariablesProvider has not changed, but by default all pairs of boolean strings are accepted when retrieving values as Bool or BoolArray. Client code can configure the provider to use a bool decoder with support only for true/false strings.
If it is more important to keep the behavior as it was, meaning default support for true/false only in the provider, then I will modify the default set for the EnvironmentVariablesProvider BoolDecoder.
Test Plan
The decoder is tested with dedicated tests, and additional tests for receiving a value from a provider were added to test the integration of BoolDecoder with EnvironmentVariablesProvider.
Code is commented, and tests are green locally.