Add missing protocol to fix Xcode 12.5 warning #7943
Conversation
Coverage ReportAffected SDKsNo changes between base commit (fc6284c) and head commit (783f185). Test Logs
|
| @@ -217,7 +217,9 @@ - (void)listenForAppCheckTokenChanges:(fbt_void_nsstring)listener { | |||
| + (id<FIRDatabaseConnectionContextProvider>) | |||
| contextProviderWithAuth:(nullable id<FIRAuthInterop>)auth | |||
| appCheck:(nullable id<FIRAppCheckInterop>)appCheck { | |||
| return [[self alloc] initWithAuth:auth appCheck:appCheck]; | |||
| return (id<FIRDatabaseConnectionContextProvider>)[[self alloc] | |||
There was a problem hiding this comment.
Thank you for catching this. I think we should add confirmation of FIRDatabaseConnectionContextProvider class to FIRDatabaseConnectionContextProvider protocol in the header instead of casting. It seems that originally the method could return different classes but it is not the case any more, so an explicit confirmation looks reasonable to me.
There was a problem hiding this comment.
@maksymmalyhin It looks like the unit tests depend on the different classes:
There was a problem hiding this comment.
Yeah, that's correct. Sorry if I was not clear. Let me try to explain my thoughts.
The warning with the original method implementation is actually correct an helpful, because the method is supposed to return an instance that conforms to the FIRDatabaseConnectionContextProvider protocol, but in fact it returns an instance of FIRDatabaseConnectionContextProvider class (the same name as the protocol to add more confusion 😄 ) which doesn't declare it's conforming to the protocol. My suggested fix for this is adding the protocol confirmation to the class, e.g. by modifying
@interface FIRDatabaseConnectionContextProvider : NSObject <FIRDatabaseConnectionContextProvider>
In this case casting won't be required and if protocol changes the compiler will suggest us changing the class (which is not the case currently). I needed to do this initially, but missed it, sorry about that.
WDYT?
There was a problem hiding this comment.
Thanks for explaining. That makes sense and is much cleaner to avoid the casts.
I was confused by the same name for both the protocol and the interface. Is that a typical style?
There was a problem hiding this comment.
I usually prefer to add Protocol suffix if there is a name conflict with a class, but here I followed the existing name convention and only applied renaming "AuthTokenProvider" -> "ConnectionContextProvider". I actually don't mind renaming the protocol.
#no-changelog