adopt sendable #218
adopt sendable #218
Conversation
tomerd
force-pushed
the
sendable
branch
3 times, most recently
from
4c4ffa5 to
c889e29
Compare
|
@swift-server-bot test this please |
| /// A metadata value which is a `String`. | ||
| /// | ||
| /// Because `MetadataValue` implements `ExpressibleByStringInterpolation`, and `ExpressibleByStringLiteral`, | ||
| /// you don't need to type `.string(someType.description)` you can use the string interpolation `"\(someType)"`. | ||
| case string(String) | ||
|
|
||
| /// A metadata value which is some `CustomStringConvertible`. | ||
| #if compiler(>=5.6) | ||
| case stringConvertible(CustomStringConvertible & Sendable) |
There was a problem hiding this comment.
Why does the value need to be Sendable here? Also this might be breaking as is.
I think to make this non breaking we need:
@preconcurrency protocol LogMetadataCustomStringConvertible: CustomStringConvertible & Sendable {}
And then we can go for:
case stringConvertible(LogMetadataCustomStringConvertible)
There was a problem hiding this comment.
Because otherwise MetadataValue cannot conform to Sendable, which in term means Logger.Metadata cannot conform to Sendable, which in the end means Logger can't conform to Sendable.
There was a problem hiding this comment.
I think to make this non breaking we need:
@preconcurrency protocol LogMetadataCustomStringConvertible: CustomStringConvertible & Sendable {}
This wouldn't work either, because this would be a new protocol. Currently, it only requires CustomStringConvertible. Maybe a typealias would work:
@preconcurrency typealias LogMetadataCustomStringConvertible = CustomStringConvertible & SendableBut I'm not sure if @preconcurrency is supported in this case.
There was a problem hiding this comment.
Because otherwise MetadataValue cannot conform to Sendable, which in term means Logger.Metadata cannot conform to Sendable, which in the end means Logger can't conform to Sendable.
Yeah, I get that. However we could also transform CustomStringConvertible to a string directly on the call-site. This would allow us to not require Sendable. However a class that might change its string representation at a later point, would always be represented by its original value, if it is added to the metadata.
This wouldn't work either, because this would be a new protocol. Currently, it only requires
CustomStringConvertible.
@ffried Very good point. Going directly to string might potentially allow us to work around the semver major here.
There was a problem hiding this comment.
I think there is more to that... stringConvertible is currently kind of an escape hatch to pass arbitrary objects to the log handlers. We use it to for example control which file a message is logged to in CocoaLumberjack's log handler. Most of these usages are probably fine with adding Sendable, though.
Also, changing the enum case this way will also result in a semver major.
There was a problem hiding this comment.
@ktoso are you saying that adding the Sendable protocol requirement will only yield warnings until Swift 6?
There was a problem hiding this comment.
That's right, I just confirmed with Doug as well. If you were seeing an error in beta-1 (maybe?), then Doug says this could have been a bug and is still just a warning nowadays and will continue to be.
There was a problem hiding this comment.
in that case I am also comfortable with this soft break.
@ffried @fabianfett @weissi @Lukasa wdyt?
There was a problem hiding this comment.
Same here. A warning is fine. 👍
It's the same as when for example a new deprecation is added. There would also be a warning then.
There was a problem hiding this comment.
I'm ok with a warning here.
|
lets take #222 first |
motivation: adjust to swift 5.6 changes: * define sendable shims for protocols and structs that may be used in async context * adjust tests * add a test to make sure no warning are emittted
|
@yim-lee @weissi @Lukasa @ktoso @fabianfett this is ready to go. ptal |
motivation: adjust to swift 5.6
changes: