New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change DefaultChannelId visibility to default. Related to [#5053] #5057
Conversation
@Scottmitch @nmittler @trustin PTAL |
/** | ||
* Create and return a new {@link ChannelId} instance. | ||
*/ | ||
public static ChannelId newInstance() { |
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.
Should return DefaultChannelId
given that this method is the factory method of DefaultChannelId
?
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.
@trustin does it make any difference though ?
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.
Yeah, I'm happy. 😆 Consistency matters!
1195718
to
66b4a7c
Compare
@@ -53,7 +53,10 @@ | |||
|
|||
private static final AtomicInteger nextSequence = new AtomicInteger(); | |||
|
|||
static ChannelId newInstance() { | |||
/** | |||
* Create and return a new {@link DefaultChannelId} instance. |
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.
I still think Returns a new ...
implies that it creates a new instance and returns it. i.e.
- 'create' is redundant.
- It should be 'returns' instead of 'return', because it's not an imperative sentence.
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.
fixed
Motivation: There is no need to make DefaultChannelId package private as it may be useful for the user. For example EmbeddedChannel allows to inject a ChannelId when it is constructed. For this case the user can just use DefaultChannelId. Modifications: Change visibility of DefaultChannelId to public. Result: It's possible to create a new instance of DefaultChannelId by the user.
66b4a7c
to
2f14d41
Compare
👏 LGTM 👍 |
Cherry-picked into 4.1 (2facb7a) |
Motivation:
There is no need to make DefaultChannelId package private as it may be useful for the user. For example EmbeddedChannel allows to inject a ChannelId when it is constructed. For this case the user can just use DefaultChannelId.
Modifications:
Change visibility of DefaultChannelId to public.
Result:
It's possible to create a new instance of DefaultChannelId by the user.