Skip to content

Conversation

@SUPERCILEX
Copy link
Collaborator

@samtstern What do you think, should we do it? On the one hand, this removes 400+ lines of code and cleans things up nicely, but on the other hand it could make it harder for people to upgrade if they were still using the deprecated stuff.

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
@SUPERCILEX
Copy link
Collaborator Author

Something else I just thought of: making any decision sets a precedent so if we don't remove stuff, people might expect deprecations to never be removed in the future.

Copy link
Contributor

@samtstern samtstern left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with you that this is a policy decision, I went and reviewed the code with intent to merge but worth discussing. @puf what do you think about removing deprecated methods when we do major version bumps?

*/
public Task<Void> signOut(@NonNull FragmentActivity activity) {
// Get Credentials Helper
GoogleSignInHelper credentialsHelper = GoogleSignInHelper.getInstance(activity);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: in the future let's try to avoid changes like this where we do a cosmetic refactor within another change. It makes the diff larger and distracts from your main purpose (which is killing deprecated code).

// Initialize SmartLock helper
CredentialTaskApi credentialHelper = GoogleSignInHelper.getInstance(activity);

return getDeleteTask(credentialHelper);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment here, I have no issue with inlining this method but it's not relevant to the title of the PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, sorry about that. These changes were introduced when deprecating the old delete(Activity) method which is why I undid them here, but I can see your point of how that's confusing.

* @deprecated Please use {@link com.firebase.ui.auth.ErrorCodes#NO_NETWORK}
**/
@Deprecated
public static final int RESULT_NO_NETWORK = com.firebase.ui.auth.ErrorCodes.NO_NETWORK;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably keep a running list of 1.x --> 2.x upgrade tips since something like this will be a little confusing. Not sure what the best format for this is, maybe an MD file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@samtstern Totally agree with you, 1113cc0 should take care of this.

…-deprecations

# Conflicts:
#	auth/src/main/java/com/firebase/ui/auth/AuthUI.java
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
…-deprecations

# Conflicts:
#	auth/src/main/java/com/firebase/ui/auth/AuthUI.java
…-deprecations

# Conflicts:
#	auth/src/test/java/com/firebase/ui/auth/AuthUITest.java
This was referenced Mar 18, 2017
…-deprecations

# Conflicts:
#	auth/src/main/java/com/firebase/ui/auth/AuthUI.java
@samtstern samtstern added this to the 2.0.0 milestone Apr 26, 2017
…-deprecations

# Conflicts:
#	auth/src/main/java/com/firebase/ui/auth/ui/idp/AuthMethodPickerActivity.java
…-deprecations

# Conflicts:
#	auth/src/main/java/com/firebase/ui/auth/ui/BaseHelper.java
@samtstern
Copy link
Contributor

@SUPERCILEX this CL has been here forever ... I've been thinking about it and 2.0 is the time to kill anything that was @Deprecated. Let's do this.

Do you have any reservations about merging this change?

@SUPERCILEX
Copy link
Collaborator Author

@samtstern Nope, LGTM! 😄

We're still keeping the deprecated setProviders method, but I think that's the right desision because we don't want to confuse or frustrate people too much.

@samtstern
Copy link
Contributor

Yeah we have to keep that method since it was never actually released as @Deprecated. But for things that have been deprecated for a release or two it's a good time to nuke em.

@samtstern samtstern merged commit b9e3f8b into firebase:version-2.0.0-dev Jun 5, 2017
@SUPERCILEX SUPERCILEX deleted the remove-deprecations branch June 5, 2017 22:16
@SUPERCILEX
Copy link
Collaborator Author

Cool beans! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants