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

Stabilize todo macro #61879

Open
wants to merge 1 commit into
base: master
from

Conversation

@stjepang
Copy link
Contributor

commented Jun 15, 2019

The todo! macro is just another name for unimplemented!.

Tracking issue: #59277

This PR needs a FCP to merge.

r? @withoutboats

@Centril

This comment has been minimized.

Copy link
Member

commented Jun 23, 2019

@BO41

BO41 approved these changes Jun 27, 2019

@withoutboats

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2019

@rfcbot fcp merge

Like Stjepan, I've been using todo, which is so much more pleasant than unimplemented (and even than panic). I like having this alias in std, and I want to be able to use it without turning on a feature flag.

@rfcbot

This comment has been minimized.

Copy link

commented Jun 27, 2019

Team member @withoutboats has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

@rfcbot concern plan-for-unimplemented

Is there a plan for what to do with unimplemented!? Is the intended long-term effect that we have both macros, or is the intended long-term goal that we deprecate unimplemented! in favor of todo!?

@stjepang

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2019

@alexcrichton Some thoughts on that by @withoutboats:

#56348 (comment)
#56348 (comment)

The short answer is that we keep unimplemented! and don't really make a distinction from todo!. Which of these two macros you use is a matter of personal taste.

@withoutboats

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2019

Is there a plan for what to do with unimplemented!? Is the intended long-term effect that we have both macros, or is the intended long-term goal that we deprecate unimplemented! in favor of todo!?

I don't think its worthwhile to deprecate unimplemented. Many people already use just panic for this purpose (including you I believe, from a conversation a long time ago). Having more than one way to do this causes no problems. We really don't need to dictate to people how they write their "panic because i didnt write this code yet" code.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

Ok, thanks for the clarification! I'm not a huge fan of having multiple ways to do things like this, but this is basically just a convenience and conversions also are a matter of taste with libstd, so it seems reasonable enough to me.

@rfcbot resolve plan-for-unimplemented

@rfcbot

This comment has been minimized.

Copy link

commented Jul 2, 2019

🔔 This is now entering its final comment period, as per the review above. 🔔

@Centril Centril modified the milestones: 1.37, 1.38 Jul 2, 2019

@JohnBSmith

This comment has been minimized.

Copy link

commented Jul 3, 2019

I think it makes sense that the two macros could have slightly different meanings, in that todo!() would indicate work in progress, but unimplemented!() would indicate something omitted intentionally.

For example, if in a textbook some detail of an algorithm does not matter for the current consideration, an unimplemented!() could be placed rather than todo!().

@smmalis37

This comment has been minimized.

Copy link
Contributor

commented Jul 4, 2019

Similar to @alexcrichton I'm not a fan of multiple ways to do something simple like this. Is there really a need for this over something like unimplemented!("TODO: Write this.") vs unimplemented!("Omitted for this example.")?

I feel like having both of these makes it harder for people to track all instances of these macros for not much gain in expressiveness as they now have to know that there's two possibilities, not just one.

@stjepang

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2019

@smmalis37

Well, I like prototyping Rust code by defining functions without implementing their bodies, leaving unimplemented!() in their place. But the thing is, if I'm writing code for myself, I tend to use panic!() instead because it's just so much easier to type than the other macro. In fact, it feels almost as if Rust is discouraging me from prototyping code.

Yes, this can be solved by external crates but I'm not going to add a dependency just for a single shorter macro. And yes, it might be solved by text editor's help but that isn't a solution everywhere (think Rust Playground).

So the bottom line is that I'm a human and having the todo!() macro would make me a happier Rust programmer. I feel the drawback of having multiple ways of doing the same thing is a small price to pay for that, but also understand others feel differently. shrug

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented Jul 4, 2019

Duplication like this makes people learning the library go through the next process

  • Should I use unimplemented or todo?
  • Google -> reading top relevant Stack Overflow question "unimplemented vs todo"
  • Top answer: they are exactly the same
  • Questions in comments - "but why???", another answer contains detailed archeology with links to this thread and other threads.
  • (Some percent of especially enthusiastic users goes back to the issue tracker or user forum to report that the library is a mess.)

Is this ok? I don't know, probably not a big deal, but I can't say I like it too much.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented Jul 4, 2019

On a related note, we really need lints with arguments enabling targeted deprecation (like #![allow(deprecated(unimplemented))]) to be able to deprecate things like unimplemented!().

@matklad

This comment has been minimized.

Copy link
Member

commented Jul 4, 2019

On a related note, we really need lints with arguments enabling targeted deprecation

While we are at it, Kotlin's replaceWith and associated tooling is also a must-have.

https://hackernoon.com/how-kotlins-deprecated-relieves-pain-of-colossal-refactoring-8577545aaed

@withoutboats

This comment has been minimized.

Copy link
Contributor

commented Jul 4, 2019

I really can't relate to these comments when std already has several synonyms for panic. I think @alexcrichton's opinion might be that none of these should have been added (since I remember him saying once that you should just use panic instead of unimplemented or unreachable), but we're well past that point.

@totorigolo

This comment has been minimized.

Copy link

commented Jul 4, 2019

I would just like to add that some of my TODO comments are more debug!("TODO this") than panic!("TODO this"). So now I will have to choose if I want to use a panic-ing macro or a noop-ing comment, depending on the context. Both have the same name, which is kinda confusing.

Fortunately, I can override it to what I want... except that I shouldn't because then it'll be hard to know it's behaviour when switching between crates.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Jul 4, 2019

To clarify my opinion, I'm not a huge fan of multiple ways to do the same thing and I think it's probably safe to say almost everyone is in roughly the same boat. That being religiously striving after this goal can and has been harmful in the past.

The standard library in general does a great job of not providing lots of tools for the same option but an area this is definitely not true is conversions. For example you can count the number of ways to convert &String to &str and it's quite large! This isn't really a bad thing though in my opinion.

I think it's of course the case that @petrochenkov's example of doom and gloom can come up, but no matter what we do that comes up. People will be mildly confused about these two macros, yes, but the whole point of them is convenience. Having slightly different conveniences isn't the end of the world and if folks are motivated enough to push it through then that seems fine by me at least.

@Amanieu

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2019

I use unimplemented! quite liberally when I am developing code. With auto-completion in vscode only 3 keystrokes are needed (un<TAB>), so I personally don't see a pressing need for a new macro which is the same as the existing one except shorter to type. Maybe the situation is different for people using other editors?

@mark-i-m

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2019

I use (neo)vim + rust-analyzer, which ATM doesn't tend to complete macro names, and the shorter macro would definitely make my life easier. Also, I already grep for TODO, when working, so having the macro with that name would be a nice bonus...

@VictorGavrish

This comment has been minimized.

Copy link

commented Jul 10, 2019

What are the actual benefits of adding this alias? I don't see it clearly enumerated anywhere.

I've skimmed through some of the previous discussion, and I think most of the arguments I've seen in favor of this change have been of the "why not?" variety. And indeed, I don't see a big drawback to doing it. In fact, it's mostly "meh" from me. But I'd argue that any addition to the standard library should have a firm "why yes" argument for it, rather than an absence of a strong resistance. There should be a certain bar for any new thing. As a casual Rust user, I've got the impression that this bar exists, and it's pretty high. But doing this change would seem inconsistent with that.

Is it just that todo!() is easier to type than unimplemented!()? That doesn't seem to be a good enough reason to add anything to the standard library. Also, I'd argue that the difference in typing (even without completion assistance) is dwarfed by the unwieldy !(). That thing takes me twice as long to type as the word itself because that's two different shift keys that I need to press and hold. Is it just that the name fits better? I think that's not very apparent and might be a matter of taste. Do we want to add different ways to do one thing just to suit people who have different tastes? Again, that doesn't seem to be like a strong enough reason to intentionally add duplication.

Just my two cents.

@mark-i-m

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2019

For me the benefits are

  1. easier to type (though I agree that !() is a pain to type). It might be just me, but one rust's great unsung features is brevity IMHO.
  2. I already grep for TODO, so this streamlines my workflow signficantly.
  3. unambiguous meaning: its clear that a todo!() is something I intend to do if others come across it, whereas panic and unimplemented do not communicate that idea.
@anowlcalledjosh

This comment has been minimized.

Copy link

commented Jul 10, 2019

fwiw, I suspect I just type weirdly, but todo!() and particularly !() are very easy for me to type – todo is a quick alternation between left and right hands, and !() is just left-shift plus ! (left hand) and () (right hand). In contrast, the first six letters of unimplemented are all typed with the right hand, which is comparatively slow and awkward.

@rfcbot

This comment has been minimized.

Copy link

commented Jul 12, 2019

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.