rewrite documentation for unimplemented! to clarify use #64726
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @rkruppe (rust_highfive has picked a reviewer for you, use r? to override) |
|
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 |
hellow554
reviewed
Sep 24, 2019
Returning |
|
@qnighy I meant regarding the example given, we could easily get it to type check without this macro. I did miss that it still allows it to compile if you try returning non-unit types. I'll edit my pull request to demonstrate this. |
|
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 |
hellow554
reviewed
Sep 24, 2019
|
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 |
|
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 |
|
r? @hellow554 |
|
Ping from triage. @rkruppe @andrewbanchich |
|
Oops, this slipped under my radar, sorry! I don't think @hellow554 has power over @bors so although their review is much appreciated they ultimately can't merge the PR. I'll take a look now. |
hanna-kruppe
suggested changes
Oct 5, 2019
There was a problem hiding this comment.
I think it's great to be more explicit about what the macro does exactly and to mention how it accepts extra arguments. Thanks for the PR and sorry again for not responding earlier.
But I'm a bit conflicted about other parts of this diff which seem neutral or negative to me (see notes below). On the other hand, some of that is subjective and I certainly don't want a bikeshed out of it. I guess I'll make some suggestions and if you agree you can incorporate them and if not I'll 8000 just shrug and merge anyway (for the subjective stuff, there is one small thing I do care strongly about).
|
Thanks! @bors r+ rollup |
|
📌 Commit b7091e4 has been approved by |
|
@rkruppe I think it still needs you to accept the changes in your review. |
Centril
added a commit
to Centril/rust
that referenced
this pull request
Oct 8, 2019
…uppe rewrite documentation for unimplemented! to clarify use The current docs for `unimplemented!` seem to miss the point of this macro. > This can be useful if you are prototyping and are just looking to have your code type-check, or if you're implementing a trait that requires multiple methods, and you're only planning on using one of them. You could also return a `()` if you just want your code to type-check. I think `unimplemented!` is useful for when you want your program to exit when it reaches an unimplemented area. I rewrote the explanation and gave examples of both forms of this macro that I think clarify its use a little better.
bors
added a commit
that referenced
this pull request
Oct 8, 2019
Rollup of 8 pull requests Successful merges: - #64726 (rewrite documentation for unimplemented! to clarify use) - #65040 (syntax: more cleanups in item and function signature parsing) - #65046 (Make `Cell::new` method come first in documentation) - #65098 (Add long error explanation for E0561) - #65150 (Suggest dereferencing boolean reference when used in 'if' or 'while') - #65154 (Fix const generic arguments not displaying in types mismatch diagnostic) - #65181 (fix bug in folding for constants) - #65187 (use 'invalid argument' for vxWorks) Failed merges: - #65179 (Add long error explanation for E0567) r? @ghost
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The current docs for
unimplemented!seem to miss the point of this macro.You could also return a
()if you just want your code to type-check.I think
unimplemented!is useful for when you want your program to exit when it reaches an unimplemented area.I rewrote the explanation and gave examples of both forms of this macro that I think clarify its use a little better.