-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Differentiate todo! and unimplemented! #67445
Conversation
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
My understanding is that @dtolnay said (#56348 (comment)) we shouldn't differentiate like this. r? @dtolnay cc @withoutboats @matklad |
In practice people use I'd prefer to just say "unimplemented is an alias for |
Re: the partial trait use-case, here's an abbreviated example from one of my projects:
This is a requirement for implementing the trait; it's a genuine
The wording is something hasn't yet been implemented; it will be implemented in the future. Conversely here's another example from the same project (again, very simplified):
At one point IIRC I actually used tl;dr:
|
We've had this debate again and again during the development of todo, and my opinion is that std should not attempt to prescribe any particular way of using the APIs, because we all will use them differently. Attempts to say that unimplemented is for a use case other than unfinished usually runs into the problem of differentiating unimplemented from unreachable. In my opinion, panicking in a case that is neither unfinished nor unreachable (e.g. any public API which panics) should use I don't understand the constant impulse to systematically prescribe every decision users make in writing code. |
Just changing unimplemented's panic message to "not implemented" instead of "not yet implemented" seems fine to me, since it just makes it more flexible (though again, I would personally just use |
@withoutboats thank you! That's a good suggestion. I'll update the PR shortly. |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@bors r+ |
📌 Commit f4d0a04 has been approved by |
/// // We have some logic here, | ||
/// // so we can use unimplemented! to display what we have so far. | ||
/// // We can add a message to unimplemented! to display our omission. |
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.
Minor typo/awkward wording. How about wording it as the following:
"We have some logic here. We can add a message to unimplemented!
to explain our omission."
@withoutboats That's fair! I didn't follow that discussion closely so I'm probably lacking context. :) |
Differentiate todo! and unimplemented! This updates the panic message and docs to make it clear that `todo!` is for unfinished code and `unimplemented!` is for partial trait or enum impls. r? @Centril
☀️ Test successful - checks-azure |
This updates the panic message and docs to make it clear that
todo!
is for unfinished code andunimplemented!
is for partial trait or enum impls.r? @Centril