-
Notifications
You must be signed in to change notification settings - Fork 25
Update package to support Swift 6.0; Drop older than 5.7 #44
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
Conversation
| #if swift(>=6.0) | ||
| public static func withValue<T>(_ value: ServiceContext?, |
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.
Actually after looking at this again this is an API break strictly speaking. We need both API shapes in Swift 6 as well since somebody could reference the method. We can just disfavour the old one
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 have mixed feelings about this rule tbh, it's somewhat superficial in practice and I think we broke it a bunch of time in various libs tbh.
I'll see if things work out with not iffing out the overload and disfavoring it, hope it just works
|
@swift-server-bot test this please |
|
@swift-server-bot test this please |
|
@swift-server-bot test this please |
…out our source compat apis using deprecated things
|
@swift-server-bot test this please |
|
Ok we're good 5.7+ I did keep the source compat version... had to disable warnings as errors to do so though |
|
|
||
| test: | ||
| <<: *common | ||
| command: /bin/bash -xcl "swift test -Xswiftc -warnings-as-errors $${FORCE_TEST_DISCOVERY-} $${SANITIZER_ARG-} $${STRICT_CONCURRENCY_ARG-}" |
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.
That's not great. Which Swift versions have warnings now? In general we aim to keep this enabled otherwise we will accumulate warnings over time
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.
6 does, literarily warning about the api this pr attempted to remove but we chose to keep it around after all. no workaround since we wanted to keep the "bad api" meh.
Tbh I'm still not convinced about keeping that api around, no one uses method references to this method :p
We could disable only on 6 heh.
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.
Disabling only 6 isn't great either because we will carry that warning forward. We might have to bite the bullet here and just say this is not an API break since we are using the @_ API here which was a problem from the beginning.
Update to support Swift 6.0 and drop old versions, same as all other server packages by now.
resolves rdar://113848512