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

Merged
merged 3 commits into from Oct 4, 2019
Merged

Stabilize todo macro #61879

merged 3 commits into from Oct 4, 2019

Conversation

ghost
Copy link

@ghost ghost 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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 15, 2019
@Centril Centril added relnotes Marks issues that should be documented in the release notes of the next release. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jun 15, 2019
@Centril Centril added this to the 1.37 milestone Jun 15, 2019
@Centril
Copy link
Contributor

Centril commented Jun 23, 2019

r? @dtolnay

@withoutboats
Copy link
Contributor

@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
Copy link

rfcbot 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.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jun 27, 2019
@Centril Centril removed the needs-fcp This change is insta-stable, so needs a completed FCP to proceed. label Jun 30, 2019
@alexcrichton
Copy link
Member

@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!?

@ghost
Copy link
Author

ghost 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
Copy link
Contributor

withoutboats 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
Copy link
Member

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 rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Jul 2, 2019
@rfcbot
Copy link

rfcbot 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
Copy link

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
Copy link
Contributor

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.

@ghost
Copy link
Author

ghost 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
Copy link
Contributor

petrochenkov 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
Copy link
Contributor

petrochenkov 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!().

@bors
Copy link
Contributor

bors commented Oct 3, 2019

⌛ Testing commit 711f673 with merge b514517937b4ae6e9c9cecf5a791b3788589ff30...

@Centril
Copy link
Contributor

Centril commented Oct 3, 2019

@bors retry rolled up.

@Centril Centril removed the I-needs-decision Issue: In need of a decision. label Oct 3, 2019
@bors
Copy link
Contributor

bors commented Oct 3, 2019

⌛ Testing commit 711f673 with merge c04eac9f5397e3e81b2645fe06f1c6d80b544303...

Centril added a commit to Centril/rust that referenced this pull request Oct 3, 2019
…oats

Stabilize todo macro

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

Tracking issue: rust-lang#59277

This PR needs a FCP to merge.

r? @withoutboats
@Centril
Copy link
Contributor

Centril commented Oct 3, 2019

@bors retry rolled up.

@bors
Copy link
Contributor

bors commented Oct 3, 2019

⌛ Testing commit 711f673 with merge 83c354a1446702165e566173a8ace58c44d73be6...

tmandry added a commit to tmandry/rust that referenced this pull request Oct 3, 2019
…oats

Stabilize todo macro

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

Tracking issue: rust-lang#59277

This PR needs a FCP to merge.

r? @withoutboats
@tmandry
Copy link
Member

tmandry commented Oct 3, 2019

@bors retry rolled up

@bors
Copy link
Contributor

bors commented Oct 3, 2019

⌛ Testing commit 711f673 with merge 8d67d343c62f4c83506dc81fd34e8107fac0b049...

tmandry added a commit to tmandry/rust that referenced this pull request Oct 3, 2019
…oats

Stabilize todo macro

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

Tracking issue: rust-lang#59277

This PR needs a FCP to merge.

r? @withoutboats
@tmandry
Copy link
Member

tmandry commented Oct 3, 2019

@bors retry, rolled up again!

@tmandry
Copy link
Member

tmandry commented Oct 3, 2019

@bors retry

bors added a commit that referenced this pull request Oct 3, 2019
Rollup of 11 pull requests

Successful merges:

 - #61879 (Stabilize todo macro)
 - #64675 (Deprecate `#![plugin]` & `#[plugin_registrar]`)
 - #64690 (proc_macro API: Expose `macro_rules` hygiene)
 - #64706 (add regression test for #60218)
 - #64741 (Prevent rustdoc feature doctests)
 - #64842 (Disallow Self in type param defaults of ADTs)
 - #65004 (Replace mentions of IRC with Discord)
 - #65018 (Set RUST_BACKTRACE=0 in tests that include a backtrace in stderr)
 - #65055 (Add long error explanation for E0556)
 - #65056 (Make visit projection iterative)
 - #65057 (typo: fix typo in E0392)

Failed merges:

r? @ghost
@bors
Copy link
Contributor

bors commented Oct 4, 2019

⌛ Testing commit 711f673 with merge 31d75c4...

@bors bors merged commit 711f673 into rust-lang:master Oct 4, 2019
@SimonSapin
Copy link
Contributor

In any future stabilization PR, please remember to make it close the corresponding tracking issue. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.