-
Notifications
You must be signed in to change notification settings - Fork 9.8k
Exposing GoogleSignIn Delegate as an interface #404
Conversation
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.
This'll need updates to the CHANGELOG and pubspec.yaml too
@@ -111,7 +148,7 @@ public void onMethodCall(MethodCall call, Result result) { | |||
* completed (either successfully or in error). This class provides no synchronization consructs | |||
* to guarantee such behavior; callers are responsible for providing such guarantees. | |||
*/ | |||
public static final class Delegate { | |||
public static final class DelegateImpl implements Delegate { |
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.
This is a breaking change. Rather than having to bump a minor version number, you could keep this Delegate
and make the interface (I can't believe I'm saying this...) IDelegate
. Then again, I hate myself for having said that, so maybe you just bump the minor version number 😄-- wdyt?
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.
As discussed offline, it's probably not worth a breaking change to change the name of the Delegate class so I renamed the interface to IDelegate.
Updated the class and interface names and increased the version number. |
@@ -111,7 +148,7 @@ public void onMethodCall(MethodCall call, Result result) { | |||
* completed (either successfully or in error). This class provides no synchronization consructs | |||
* to guarantee such behavior; callers are responsible for providing such guarantees. | |||
*/ | |||
public static final class Delegate { | |||
public static final class Delegate implements IDelegate { |
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.
Add @Override
annotations on the interface methods.
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.
Done
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.
LGTM with one comment.
This allows for the creation of testing versions of this delegate for any other plugin that wants to use it.