-
Notifications
You must be signed in to change notification settings - Fork 73
Deny todo!() in tidy #999
Copy link
Copy link
Open
Labels
T-compilerAdd this label so rfcbot knows to poll the compiler teamAdd this label so rfcbot knows to poll the compiler teamfinal-comment-periodThe FCP has started, most (if not all) team members are in agreementThe FCP has started, most (if not all) team members are in agreementmajor-changeA proposal to make a major change to rustcA proposal to make a major change to rustc
Metadata
Metadata
Assignees
Labels
T-compilerAdd this label so rfcbot knows to poll the compiler teamAdd this label so rfcbot knows to poll the compiler teamfinal-comment-periodThe FCP has started, most (if not all) team members are in agreementThe FCP has started, most (if not all) team members are in agreementmajor-changeA proposal to make a major change to rustcA proposal to make a major change to rustc
Type
Fields
Give feedbackNo fields configured for issues without a type.
Proposal
In tidy, we reject code with comments of the form
// TODO.*. Those are intended to be fixed before merging PRs. I like this behavior, it means you can set yourself tasks that you definitely cannot forget before merging. Curiously, we don't do the same for thetodo!()macro, even though I'd like to use it for the same purpose. For example, in a match, to signify that I still want to implement that branch before merging.I propose to also deny
todo!()in tidy. As for when something is really left as TODO, and intended to be so after a PR is merged, I propose usingunimplemented!instead to make it explicit that the branch is intended to be fixed in a later PR, not this one.There are currently about 40 uses of
todo!()in the compiler. Implementing the tidy rule is ~5 lines and the fixes are mostly trivial. Yet, I still thought it worth an MCP since this is a procedure everyone would have to follow if we accept.Process
The main points of the Major Change Process are as follows:
@rustbot secondor kickoff a team FCP with@rfcbot fcp $RESOLUTION.You can read more about Major Change Proposals on forge.