fix: add missing switch cases and update compiler check #457
Conversation
packages/sign_in_with_apple/sign_in_with_apple/darwin/Classes/SignInWithAppleError.swift
Show resolved
Hide resolved
|
@juliansteenbakker Thanks, I'll look into this and see what we can do. Could you please rebase this onto |
.github/workflows/dart.yml
Outdated
| @@ -61,15 +61,17 @@ jobs: | |||
| matrix: | |||
| os: [macos-13, macos-14, macos-15] | |||
| package: [sign_in_with_apple/sign_in_with_apple] | |||
| xcode: ["14.3.1", "15.2", "16.0"] | |||
| xcode: ["14.3.1", "15.2", "16.1"] | |||
There was a problem hiding this comment.
Personally I would like to use at least 16.1 and 16.2 (where the new case was added and what you check with compiler(>=6.0.3), right?).
I don't think we can expect users (and their CI) to always use the very latest Xcode version shortly after release, so we have to be sure to support the currently expected ones.
There was a problem hiding this comment.
These changes were not part of my original PR, but changes already merged in #456.
I have removed these changes from my PR, so now it should be correct.
As for the check, i think it would be good to also check for 16.2, so if wanted, i can add that to my PR.
There was a problem hiding this comment.
As for the check, i think it would be good to also check for 16.2, so if wanted, i can add that to my PR.
Yes, that's what I would also suggest, given that the change you added (with that specific compiler version) will only be included in 16.2.
juliansteenbakker
force-pushed
the
fix/switch-case-exhaustive
branch
from
089071f to
41c3155
Compare
| @@ -107,15 +107,21 @@ public enum SignInWithAppleError { | |||
| errorCode = "authorization-error/notHandled" | |||
| case .failed: | |||
| errorCode = "authorization-error/failed" | |||
| #if (os(iOS) && swift(>=5.5)) || (os(macOS) && swift(>=5.5.1)) | |||
| #if compiler(>=5.5) | |||
There was a problem hiding this comment.
Was this change needed? The old comment refers to this being shipped in different versions for iOS and macOS.
But overall I would not mind to change this, as we only support Xcode 14.3.1 (with Swift 5.8), so this will work on either OS with this check and that macOS version.
Just generally of the "don't fix what's not broken" camp here, as we don't know if there is not some user relying on this behavior because of a certain old Xcode version (though by now there might be 3 other changes that would've long prevented that one from using the latest version of this package, as we can not test every single combination).
Fixes warning during build about switch case not being exhaustive