Skip to content

core: clean up code deprecated in 1.1#2678

Merged
zhangkun83 merged 1 commit intogrpc:masterfrom
lukaszx0:clean-up-deprecations2
Feb 21, 2017
Merged

core: clean up code deprecated in 1.1#2678
zhangkun83 merged 1 commit intogrpc:masterfrom
lukaszx0:clean-up-deprecations2

Conversation

@lukaszx0
Copy link
Copy Markdown
Contributor

Since we're on 1.2 train, we can clean this up now :)

@lukaszx0 lukaszx0 force-pushed the clean-up-deprecations2 branch from 735cc32 to 7f8de7c Compare January 31, 2017 06:47
Copy link
Copy Markdown
Contributor

@carl-mastrangelo carl-mastrangelo left a comment

Choose a reason for hiding this comment

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

Unfortunately, ClientAuthInterceptor is widely used internally.

@lukaszx0
Copy link
Copy Markdown
Contributor Author

What's the deprecation policy then? (ie. when we're free to clean up stuff?) Also, ClientAuthInterceptor seems like the one which is very easily "backportable" - just copy it into your repo internally, even leaving original package name.

@ejona86
Copy link
Copy Markdown
Member

ejona86 commented Jan 31, 2017

As far as our deprecation process, we are free to delete it in 1.1 because it was deprecated in 1.0. But since we've not migrated users over to the new thing yet in our codebase we've not made sure the process is smooth. So it's easy to just leave it there for now and remove it once we're sure the next thing and process is ready.

@lukaszx0
Copy link
Copy Markdown
Contributor Author

That's fair, and I don't have the intention to push for it, but wouldn't it be better to get rid of it now, and motivate users to make a switch before 1.2 will be release? (which will take some time). That'd work unless you guys aren't building against HEAD, obviously, in which case I'll revert deletion of ClientAuthInterceptor :)

@lukaszx0 lukaszx0 force-pushed the clean-up-deprecations2 branch from 7f8de7c to 276b68c Compare February 5, 2017 00:01
@lukaszx0
Copy link
Copy Markdown
Contributor Author

lukaszx0 commented Feb 5, 2017

@ejona86 @carl-mastrangelo I've restored the ClientAuthInterceptor.

* @deprecated {@link Object#equals(Object)} is not supported on TolerantDeadlineComparison
* If you meant to compare deadlines, use {@link #of(Deadline)} instead.
*/
@Deprecated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is actually here to scare people away from it. I think the deprecated is present to make IDEs catch on more quickly that they shouldn't be using it. The exception is to enforce it. Truth makes it easy to accidentally call:

assertAbout(DeadlineSubject.deadline()).that(myDeadline).equals(otherDeadline);

vs.

assertAbout(DeadlineSubject.deadline()).that(myDeadline).IsEqual(otherDeadline);

The first asserts that the DeadlineSubject itself is equal to the deadline, where as the latter compares the subject to the object.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah, makes sense. I've added comment clarifying that.

*
* @deprecated Use {@link #maxInboundMessageSize} instead
*/
@Deprecated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We still use this method internally, and while it can be removed per the compat guidelines, we wouldn't be able to sync gRPC internally. We find a fair number of issues when doing the sync, so removing this now would cause more issues than it fixes.

I am undertaking the fixing of users, but it takes time.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Reverted.

Add comment claryfing deprecation in DeadlineSubject
@lukaszx0
Copy link
Copy Markdown
Contributor Author

@carl-mastrangelo is it okay now, in this form?

Copy link
Copy Markdown
Contributor

@carl-mastrangelo carl-mastrangelo left a comment

Choose a reason for hiding this comment

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

LGTM.

@zhangkun83 Want to double check it?

@zhangkun83 zhangkun83 changed the title Clean up code deprecated in 1.1 core: clean up code deprecated in 1.1 Feb 21, 2017
@zhangkun83 zhangkun83 merged commit 70b482a into grpc:master Feb 21, 2017
@lock lock bot locked as resolved and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants