-
Notifications
You must be signed in to change notification settings - Fork 25
SCO-0004: Add ExpressibleByConfigInt #135
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
| extension Duration: ExpressibleByConfigInt { | ||
| // swift-format-ignore: AllPublicDeclarationsHaveDocumentation | ||
| public init?(configInt: Int) { | ||
| self = .seconds(configInt) |
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.
I'll play devil's advocate here. I understand that the standard time unit is the second, but for example in STOMPNIO I get a Duration type from config, except that in the STOMP protocol the time unit used is milliseconds (this is accurately documented in DocC, of course).
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 code matches my understanding of the issue — specifically, that it allows directly initializing Duration using the default unit.
Do you see any issues with this approach, especially regarding unit assumptions, @czechboy0?
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.
It's a good question, basically: is there a single obvious conversion from Int -> Duration, or are there multiple equally expected ones?
I'd still argue that seconds, being an SI unit, is the most obvious projection from Int. And if you want Duration from milliseconds, you just need to wrap the call to int(forKey:) and construct it manually. This part could, in the future, be made easier with: #22.
3947083 to
fe4e40c
Compare
# Conflicts: # Sources/Configuration/Documentation.docc/Guides/Choosing-reader-methods.md #Updated my branch comment to follow text changes in main
…essibleByConfigString
|
Thanks @Adobels - the PR looks basically ready, can you write up a short proposal for it as per https://swiftpackageindex.com/apple/swift-configuration/1.0.0/documentation/configuration/proposals - won't need to be long, we just want to give the community a chance to chime in. Thanks! 🙏 |
I'm ready for a review |
|
This proposal is now In Review until January 25th: https://forums.swift.org/t/proposal-sco-0004-add-expressiblebyconfigint/84109 |
Motivation
Closes #96
Modifications
Add the ExpressibleByConfigInt protocol, mirroring ExpressibleByConfigString.
Add an ExpressibleByConfigInt extension for the Swift.Duration type.
Result
Convert configuration values of int and intArray to a type that conforms to ExpressibleByConfigInt, or to an enumeration with Int raw values.
Configuration values expressed in seconds can be converted directly to the Swift.Duration type.
Test Plan
Add tests based on the existing ExpressibleByConfigString tests.