Skip to content
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

channelRead0 is a stupid name (SimpleChannelInboundHandler) #1590

Closed
md-5 opened this issue Jul 17, 2013 · 20 comments
Closed

channelRead0 is a stupid name (SimpleChannelInboundHandler) #1590

md-5 opened this issue Jul 17, 2013 · 20 comments
Assignees
Labels
Milestone

Comments

@md-5
Copy link
Contributor

md-5 commented Jul 17, 2013

We had messageRecived, now we have channelRead0... the name seems REALLY internal and specific.

@trustin
Copy link
Member

trustin commented Jul 17, 2013

I agree. Couldn't find a good alternative though. Now that the public API is not easy to change, we have to live with it. :-/

Sent from a mobile device.
https://twitter.com/trustin
https://twitter.com/trustin_ko
https://twitter.com/netty_project

-----Original Message-----
From: md-5 notifications@github.com
To: netty/netty netty@noreply.github.com
Sent: Wed, 17 Jul 2013 11:08 AM
Subject: [netty] channelRead0 is a stupid name (SimpleChannelInboundHandler) (#1590)

We had messageRecived, now we have channelRead0... the name seems REALLY internal and specific.


Reply to this email directly or view it on GitHub:
#1590

@md-5
Copy link
Contributor Author

md-5 commented Jul 17, 2013

Sorry, cross posted across issues,surely messageReceived would be adequate? Maybe for 4.1?

@He-Pin
Copy link
Contributor

He-Pin commented Jul 17, 2013

messageReceived or channelMessageRead both okay,the channelRead0 style is used in internal.

@md-5
Copy link
Contributor Author

md-5 commented Jul 17, 2013

the channelRead0 style is used in internal.
Yeah thats my point :)

@He-Pin
Copy link
Contributor

He-Pin commented Jul 17, 2013

@md-5 I don't like that too,for trustin,i don't think that many user has convert to netty 4 and treat netty4.0.0-final as final.
and if still keep the channelRead0,then will have an endless pain for the user~:P
I promote +1 to change it.

@md-5
Copy link
Contributor Author

md-5 commented Jul 17, 2013

As someone who has used Netty 4 in 2 production applications since Alpha1, I feel that the jump to Final was WAY too quick given the scope of the changes since ~CR8 (which I currently use) -> Final. I feel that there should have been more community involvement before it was given the OK.

@md-5
Copy link
Contributor Author

md-5 commented Jul 17, 2013

Also I vote for messageReceived so it matches everything else

@He-Pin
Copy link
Contributor

He-Pin commented Jul 17, 2013

@md-5 as I can see,no Chinese coder will send the full request ,open issue or tracking it,even most mobile game company using netty,them like to use QQ group to exchange their experience,so I think,netty is really need more community involvement.
all in all,I vote for messageReceived cause it's more unintuitive just like netty3.

First the channelInboundUpdated then MessageReceived in CR5 and now channelRead;
cause there is an channelRead method in the ChannelInboundHandlerAdapter,so trustin make an hard
decision and choose channelRead0 to make it different.

@jpinner
Copy link

jpinner commented Jul 17, 2013

@trustin & @normanmaurer -- In the previous CRs, was the object autoreleased after the messageReceived call?

If the answer is yes and the behavior is the same, then I am OK with renaming this method back to messageReceived if that's what you decide to do.

If the answer is no and the autorelease behavior is new, then I don't think we should reuse the old name with different behavior.

@md-5
Copy link
Contributor Author

md-5 commented Jul 17, 2013

@jpinner
SimpleChannelInboundHandler will autoRelease by default.

@normanmaurer
Copy link
Member

@md-5 @trustin @jpinner if we really want to get rid of channelRead0(..) we should do the following:

  • deprecated SimpleChannelInboundHandler
  • Add a new handler which does exactly the same as SimpleChannelInboundHandler but use different methodname then channelRead0(...). We could even let it just extend SimpleChannelInboundHandler and surpress the deprecation warning
  • Remove SimpleChannelInboundHandler in 4.1.0 or 4.2.0

@jongwook
Copy link

I completely agree with the title. I almost gave up the migration when I saw the name.

@cgbystrom
Copy link
Contributor

I found the name a bit confusing as well.

@trustin
Copy link
Member

trustin commented Nov 2, 2013

Instead of introducing another handler class with an obscure name, I'd like to just break the compatibility because we are working on 5.0 already.

@normanmaurer
Copy link
Member

@trustin you are refer to my comment here ?

trustin added a commit that referenced this issue Nov 2, 2013
@ghost ghost assigned trustin Nov 2, 2013
@trustin
Copy link
Member

trustin commented Nov 2, 2013

Yeah.

@trustin trustin closed this as completed Nov 2, 2013
@normanmaurer
Copy link
Member

Sure we can do this… What you think would be a good one ?

Am 02.11.2013 um 12:04 schrieb Trustin Lee notifications@github.com:

Yeah.


Reply to this email directly or view it on GitHub.

@trustin
Copy link
Member

trustin commented Nov 2, 2013

Ugh, I meant it isn't worth doing that at this stage. As you see I just renamed channelRead0 to messageReceived, and updated the Javadoc.

@normanmaurer
Copy link
Member

@trustin I see... works for me ;)

@avinayak
Copy link

thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

8 participants