Downgrade to Swift 5.0 #128
Conversation
slashmo
force-pushed
the
fix/swift-5-support
branch
3 times, most recently
from
dd96d4c to
71385b4
Compare
slashmo
force-pushed
the
fix/swift-5-support
branch
from
71385b4 to
f82b55d
Compare
| /// This is a "magic value" that is used to enable the KeyPath based accessors to specific attributes. | ||
| /// This value will never be stored or returned, and any attempt of doing so would WILL crash your application. | ||
| case __namespace | ||
| #endif |
There was a problem hiding this comment.
not sure we should do this about the enum case; keep it in, even if we dont use it.
Also, we should see if we can remove this __namespace -- it's a bit of a hack.
Can you make a ticket for it? "See if we can remove the case __namespace in attributes"?
There was a problem hiding this comment.
I left this out on purpose because it's not being used anywhere but the 5.2 "magic". What would be the benefit of leaving it in for < 5.2?
There was a problem hiding this comment.
If you have a library that supports all swift versions, and they happen to switch over the attributes enum, then they'd be unable to write ONE switch implementation that works on all compiler versions without using default since they have to handle __namespace on 5.2+ and they couldn't handle it on previous versions because it's compiled out.
We should keep enums always the same across all supported compiler versions
| @@ -353,7 +363,33 @@ public struct SpanAttributes: Equatable { | |||
| where Namespace: SpanAttributeNamespace { | |||
| SpanAttribute.__namespace[keyPath: dynamicMember] | |||
| } | |||
| } | |||
| #else | |||
There was a problem hiding this comment.
why the entire type? can we not #if only the "offending" subscript?
There was a problem hiding this comment.
It's slightly more than just the subscript. Prior to 5.2 we e.g. won't need @dynamicMemberLookup on SpanAttributes, right? Also, subscript(_ name: String) in the case of < 5.2 doesn't need to handle __namespace.
| @@ -128,9 +129,11 @@ final class SpanTests: XCTestCase { | |||
| XCTAssertEqual(attributes.name, SpanAttribute.string("kappa")) | |||
| XCTAssertEqual(attributes.name, "kappa") | |||
| XCTAssertEqual(attributes.sampleHttp.statusCode, 200) | |||
| #endif | |||
There was a problem hiding this comment.
#else
print some info about shy this is skipped?
#endif
There was a problem hiding this comment.
What do you mean by "print"? Something like #warning or an actual runtime print, or simply a comment?
There was a problem hiding this comment.
I meant print("oh oh") to be honest :)
In my own test suites I usually print something in yellow for such tests...
but either's fine really
slashmo
force-pushed
the
fix/swift-5-support
branch
from
efd52c2 to
d87b196
Compare
|
@ktoso Feel free to squash this one if accepted. Had some smaller commits fixing up unit tests / CI. |
|
This is LGTM once the enum __namespace is not excluded 👍 |
Adds support for Swift 5.0 & Swift 5.1 which is necessary for being integrated by other libraries that still support these versions. The CI now executes tests in parallel on 5.0.3, 5.1, and 5.2.