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

First question mark in doctest #61990

Merged
merged 1 commit into from
Jul 7, 2019
Merged

Conversation

llogiq
Copy link
Contributor

@llogiq llogiq commented Jun 20, 2019

We have had ? for Results in doctests for some time, but so far haven't used them in doctests. With this PR, I want to start the de-unwrapping of doctests – and the discussion on where to do so.

There is one downside, which is that the code can no longer be copied into a plain main() method, on the other hand, there should be a workable error if one does this.

@rust-highfive
Copy link
Collaborator

r? @bluss

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 20, 2019
@llogiq
Copy link
Contributor Author

llogiq commented Jun 27, 2019

r? @QuietMisdreavus

@llogiq
Copy link
Contributor Author

llogiq commented Jul 2, 2019

ping @QuietMisdreavus

@QuietMisdreavus
Copy link
Member

cc @rust-lang/docs

We currently use an explicit fn main -> std::io::Result<()> in the io/fs doctests. If i remember right, we made the function signature explicit to reduce confusion because people were copying them into main and getting errors, as you implied. I'm not totally sure we should be using this feature for that reason, and instead write an explicit signature for fn main.

@GuillaumeGomez
Copy link
Member

I strongly approve this! It's removing unwrap calls from example codes and also make them easier to read.

@llogiq
Copy link
Contributor Author

llogiq commented Jul 3, 2019

So until #61277 is fixed, I'll add the explicit fn main() -> Result<..> but hide it, and remove all of them afterwards.

@llogiq
Copy link
Contributor Author

llogiq commented Jul 3, 2019

No, wait, that won't help with copying. On the other hand, adding fn main() probably won't make the example more readable either (unless there's a lot of unwraps).

@QuietMisdreavus
Copy link
Member

IMO, for consistency, we should add the explicit fn main and display it, since that's what we already do in the std documentation. Showing the explicit return type adds valuable context about the use of the ? operator, and hiding it only causes confusion as i mentioned earlier. It's okay to let the example get a little verbose.

@llogiq
Copy link
Contributor Author

llogiq commented Jul 3, 2019

Ok then. I'll update the PR shortly.

@llogiq
Copy link
Contributor Author

llogiq commented Jul 3, 2019

@QuietMisdreavus updated. Does this look OK to you?

Copy link
Member

@QuietMisdreavus QuietMisdreavus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks!

@QuietMisdreavus
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jul 7, 2019

📌 Commit ee05fc8 has been approved by QuietMisdreavus

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 7, 2019
Centril added a commit to Centril/rust that referenced this pull request Jul 7, 2019
…sdreavus

First question mark in doctest

We have had `?` for `Result`s in doctests for some time, but so far haven't used them in doctests. With this PR, I want to start the de-`unwrap`ping of doctests – and the discussion on where to do so.

There is one downside, which is that the code can no longer be copied into a plain `main()` method, on the other hand, there should be a workable error if one does this.
bors added a commit that referenced this pull request Jul 7, 2019
Rollup of 4 pull requests

Successful merges:

 - #61990 (First question mark in doctest)
 - #62379 (Add missing links in Option documentation)
 - #62438 (rustbuild: Cleanup global lint settings)
 - #62455 (name the trait in ambiguous-associated-items fully qualified suggestion)

Failed merges:

r? @ghost
@bors bors merged commit ee05fc8 into rust-lang:master Jul 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants