Skip to content

Conversation

m20082008m
Copy link
Contributor

No description provided.

import io.servicecomb.foundation.vertx.AsyncResultCallback;

public interface TcpResonseCallback extends AsyncResultCallback<TcpData> {
public interface TcpResponseCallback extends AsyncResultCallback<TcpData> {
Copy link
Member

Choose a reason for hiding this comment

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

We need to still keep the old wrong API with deprecated annotation to warn the user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed, thanks for pointing out.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.007%) to 87.066% when pulling c7b40b9 on m20082008m:fixed into a195d21 on ServiceComb:master.

@WillemJiang WillemJiang changed the title fixed typos fixed typos of TcpResonseCallback Nov 27, 2017
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 86.863% when pulling f0c6fd5 on m20082008m:fixed into a195d21 on ServiceComb:master.

@wujimin
Copy link
Contributor

wujimin commented Nov 27, 2017

keep the old interface
and create new interface, but all input parameter changed to new interface

i think it's not compatible.
but maybe nobody do this type extension now?
we change the name directly?

@WillemJiang
Copy link
Member

@wujimin You are right, the other method is public which could affect to the user.
If the send method is lower level API, may be we can add some words on the release note to remind user for that.

Copy link
Contributor

@liubao68 liubao68 left a comment

Choose a reason for hiding this comment

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

This is fine to me. We do not need to do much compatible things for typos, because this will give compile error to developers, and they can easily discover and change their code. Maybe show this in RN is enough。

@liubao68 liubao68 merged commit 4f64fb4 into apache:master Nov 29, 2017
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.

5 participants