Skip to content

libstd: replace all try! with ? in documentation examples #38648

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

Conversation

utkarshkukreti
Copy link
Contributor

See #38644.

For the record, I used the following Perl one-liner and then manually fixed a couple of things it got wrong:

$ perl -p -i -e 's#(///.*)try!\((.*)\)#$1$2?#' src/libstd/**/*.rs

@rust-highfive
Copy link
Contributor

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@utkarshkukreti
Copy link
Contributor Author

r? @steveklabnik

@steveklabnik
Copy link
Member

/cc @rust-lang/docs , I would like more than one person to review this PR, as it's very long and tedious, which means that we're likely to miss something.

@@ -205,16 +205,15 @@
//!
//! Last, but certainly not least, is [`io::Result`]. This type is used
//! as the return type of many `std::io` functions that can cause an error, and
//! can be returned from your own functions as well. Many of the examples in this
//! module use the [`try!`] macro:
//! can be returned from your own functions as well.
//!
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not replace Many of the examples in this module use the [try!] macro with Many of the examples in this module use the [? operator] (and link to the Syntax Index, probably)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think that information is really relevant in this module documentation, but I'd be happy to add what you suggested if others think the same. @steveklabnik?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think that documenting try! or ? is needed here, but I don't feel strongly.

Copy link
Member

Choose a reason for hiding this comment

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

My inclination is to say that this PR should be about a fairly mechanical translation, so keeping it here is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@Stebalien
Copy link
Contributor

I'd suggest manually converting many of the multi-line let a = try!(foo()); let b = try!(bar(a)); ... blocks of code with a()?.b()?.c()? expressions. This was a major motivation for introducing the question mark operator.

@utkarshkukreti
Copy link
Contributor Author

@Stebalien I started doing that but found many cases in which I'm not sure if a single line would be clearer for the reader (I personally would write most of these in a single line in my code). For example, what do you (and others as well) think about converting these 2 cases into single lines:

let mut f = File::create("foo.txt")?;
f.write_all(b"Hello, world!")?;

-> File::create("foo.txt")?.write_all(b"Hello, world!")?;

and

let mut f = File::create("foo.txt")?;
f.set_len(10)?;

-> File::create("foo.txt")?.set_len(10)?;?

@steveklabnik
Copy link
Member

or example, what do you (and others as well) think about converting these 2 cases into single lines:

I tend to generally write them as two lines.

That said, I think it's best to not mess too much with the formatting of the examples here, and just worry about the transformation. So let's :shipit:

Thank you!

@bors: r+

@josephpd3
Copy link

As someone new to the syntax, I like the decision to keep things unchained for these examples. It seems clearer visually, given the context. Looks good @utkarshkukreti!

@@ -250,7 +250,7 @@
//! [`println!`]: ../macro.println.html
//! [`Lines`]: struct.Lines.html
//! [`io::Result`]: type.Result.html
//! [`try!`]: ../macro.try.html
//! [`?` operator]: ../../book/syntax-index.html
Copy link
Member

Choose a reason for hiding this comment

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

its sort of a shame that one cannot link right to the ? operator within the index, but AFAICT there is no anchor for it specifically. (You could link to https://doc.rust-lang.org/stable/book/syntax-index.html#operators-and-symbols but I'm not sure if that pays off.)

@pnkfelix
Copy link
Member

pnkfelix commented Jan 3, 2017

r=me, in case you still wanted a secondary reviewer @steveklabnik

@alexcrichton alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jan 19, 2017
@bors
Copy link
Collaborator

bors commented Jan 22, 2017

☔ The latest upstream changes (presumably #39176) made this pull request unmergeable. Please resolve the merge conflicts.

@frewsxcv
Copy link
Member

It looks like the bors approval didn't go through for this PR for some reason (hence it not being merged yet). @utkarshkukreti If you get some time to address the merge conflicts, we can try again. Sorry about that

@utkarshkukreti utkarshkukreti force-pushed the question-mark-in-libstd-documentation-examples branch from 17575ae to 19724d3 Compare January 22, 2017 15:40
@utkarshkukreti
Copy link
Contributor Author

@frewsxcv rebased!

@frewsxcv
Copy link
Member

Thanks!

@bors r=pnkfelix,steveklabnik,frewsxcvx

@bors
Copy link
Collaborator

bors commented Jan 22, 2017

📌 Commit 19724d3 has been approved by pnkfelix,steveklabnik,frewsxcvx

@bors
Copy link
Collaborator

bors commented Jan 22, 2017

⌛ Testing commit 19724d3 with merge b79081c...

bors added a commit that referenced this pull request Jan 22, 2017
…tation-examples, r=pnkfelix,steveklabnik,frewsxcvx

libstd: replace all `try!` with `?` in documentation examples

See #38644.

For the record, I used the following Perl one-liner and then manually fixed a couple of things it got wrong:

    $ perl -p -i -e 's#(///.*)try!\((.*)\)#$1$2?#' src/libstd/**/*.rs
@bors
Copy link
Collaborator

bors commented Jan 22, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: pnkfelix,steveklabnik,frewsxcvx
Pushing b79081c to master...

@bors bors merged commit 19724d3 into rust-lang:master Jan 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

10 participants