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

Renaming Token #905

Closed
Thomasdezeeuw opened this issue Jan 6, 2019 · 15 comments
Closed

Renaming Token #905

Thomasdezeeuw opened this issue Jan 6, 2019 · 15 comments
Labels
rfc Request for comments.
Milestone

Comments

@Thomasdezeeuw
Copy link
Collaborator

Thomasdezeeuw commented Jan 6, 2019

Branching of #901. Renaming Token to EventedId event::Id (in a new event module`).

@Thomasdezeeuw
Copy link
Collaborator Author

@carllerche wrote:

Token -> EventId.

I'm OK renaming it, but generally idiomatic rust tries to avoid "stuttering". So, if EventId lives in an event module, it could just be named Id and referenced as event::Id.

I agree with the stuttering, something that is anti-pattern in Go as well, but I'm not sure about Rust. In Go you can only import the package/namespace, i.e. use mio::event, but in Rust that is not the case. I for example would use mio::event::EventedId and then use EventedId later on in the code.

I think is even clearer when importing/use more then one type, e.g. use mio::event::{Events, Event, EventedId}, event::Event stutters as well (though I'm not suggesting to change the name). I guess it would come down to how the type is used in practice.

@carllerche
Copy link
Member

Related: rust-lang/api-guidelines#66

I'm fine w/ whatever.

@carllerche
Copy link
Member

Specifically: rust-lang/api-guidelines#66 (comment)

@Thomasdezeeuw
Copy link
Collaborator Author

Specifically: rust-lang-nursery/api-guidelines#66 (comment)

I agree with this comment, and I think event is short enough to write. In fact EventedId and event::Id have the same length, so no different there. I don't really feel that strongly either way, if we always use event::Id in documentation/examples etc.

Maybe we should have a "Looking for feedback" label so other we can get some more feedback from users?

@carllerche carllerche added the rfc Request for comments. label Jan 11, 2019
@carllerche
Copy link
Member

I created an RFC label.

@uazu
Copy link
Contributor

uazu commented May 9, 2019

What's the motivation for changing it? Token works for me as it is short and distinct, and easy to talk about. Also it fits the meaning of a 'token', i.e. something short standing in for something else bigger. EventedId is quite weird to pronounce and not that distinct to the eye. (Evented itself also seems weird to me, because there is no verb "to event", and they are referred to as handles in the API.)

@Thomasdezeeuw
Copy link
Collaborator Author

@uazu The first line of documentation of Token reads: "Associates readiness notifications with Evented handles." To me this doesn't say token, but identifier hence token -> id. But @carllerche made a good point that event::Id was better then EventedId, which I'm in favour of as well. I've also updated the first comment to reflect this.

With this change you can use event::Id or even Id, which is even shorter then Token. I also think the pronunciation is also solved then. @uazu Would you still be in favour of Token of event::Id?

@uazu
Copy link
Contributor

uazu commented May 9, 2019

The whole thing is a muddle of different terms right now, but fortunately it is a very small API so it's workable. However I find Token easy to understand, and Evented not easy to understand. I guess that Token lead to the name tokio, so it has history. Yes, everything ID-like could be an ID, but "token" also works logically, and having less tokens around than IDs makes it easier to discuss. Also this is the ID of an event source, not of an individual event, so I'm not sure that event::Id conveys the meaning any better.

I was just looking at what these things are called in the underlying sys calls. Token is just called "data" in epoll and kqueue, which is less distinct than either ID or Token. Evented is just a "file descriptor". It appears that "handle" is not used now in the man pages, but is still commonly understood, and Windows uses that. UNIX has a lot of IDs that aren't called IDs -- so I think there is precedent for using other terms than ID (like "number" or "descriptor" or "key" or "handle").

Personally, I feel that having distinct terms within a local region of code helps understanding and discussion. But really this is very localized. It doesn't affect a lot of code, because the Tokens get wrapped in higher-level abstractions. Still, the first thing I learned about mio was that is was token-based. It seems strange to drop one of the clearer and more distinct names for something more generic.

@carllerche
Copy link
Member

@uazu What you are saying does resonate with me.

Do you have thoughts about what would be a better name than Evented?

@uazu
Copy link
Contributor

uazu commented May 9, 2019

@carllerche I hope I wasn't being too critical. For Evented, some possibilities:

  • Source or EventSource could express that it is logically a source of readiness events.
  • Handle could express that it wraps a handle to an underlying OS object, and matches existing language surrounding Evented
  • Registrable could express something that can be registered with poll, and matches the trait methods

Those are the best ideas I've found.

@ghost
Copy link

ghost commented May 9, 2019

I was also confused by Evented on my first encounter - it sounds like something that has been "evented", while it really is something that is a source of events.

My personal favorite is thus Source. Registrable sounds okay too.

@Thomasdezeeuw
Copy link
Collaborator Author

I also agree with Source, maybe in th event module, so you can write event::Source? If people agree I can make a pr.

@Thomasdezeeuw Thomasdezeeuw added this to the v0.7 milestone Jun 17, 2019
@carllerche
Copy link
Member

I think I like Source, but let me ping a few more people to get feedback.

@Thomasdezeeuw
Copy link
Collaborator Author

I think for now it's best to leave the Token name and instead rename Evented (#1003).

@carllerche
Copy link
Member

Sounds good to me 👍 I will close this.

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

No branches or pull requests

3 participants