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

Improve documentation for rand::random #15746

Closed
wants to merge 1 commit into from

Conversation

steveklabnik
Copy link
Member

This is now linked to in the guide, so I want to make sure it's good. This
adds a bit more explanation, and brings usage in line with current good style.

@steveklabnik
Copy link
Member Author

/cc @huonw

/// use std::rand::random;
/// use std::rand;
///
/// let x = rand::random();
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason you removed the if random() part of the example as well?

Also, is it worth having a comment here indicating the inference is running "backwards"?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for re-addition of if random()

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, without explanation, I had no idea why it was even there. Now that you've mentioned it, it's to demonstrate that it can generate a boolean? I'd be about putting that back with a comment explaining that.

Copy link
Member

Choose a reason for hiding this comment

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

It was there to demonstrate a variety of ways in which the type of the return value can be inferred.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, this makes sense looking back. Without context, I had no idea.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, other people have actually had confusion too for the similar example in the main module doc string: http://stackoverflow.com/q/24153311/1256624

@steveklabnik
Copy link
Member Author

Added the if back in.

I don't think it's worth talking as much about the inference because that's how inference works in general, and this is an API doc.

@huonw
Copy link
Member

huonw commented Jul 18, 2014

I don't think it's worth talking as much about the inference because that's how inference works in general, and this is an API doc.

I was thinking in the context of the first doc page the people land on from the new guide; and, this form of "raw" return type deduction isn't super-common, so having more examples is better than having fewer, IMO.

@steveklabnik
Copy link
Member Author

That's true. Let's see how it goes. In the guide, I explicitly talk about the let x: int = foo() vs let x = foo::<int>() case...

@steveklabnik
Copy link
Member Author

Whoops. Fixed.

This is now linked to in the guide, so I want to make sure it's good. This
adds a bit more explanation, and brings usage in line with current good style.
@steveklabnik
Copy link
Member Author

😢

I ❤️ you @bors. pleeeeease do it this time.

@steveklabnik
Copy link
Member Author

@alexcrichton looks like @bors needs a retry

bors added a commit that referenced this pull request Jul 19, 2014
This is now linked to in the guide, so I want to make sure it's good. This
adds a bit more explanation, and brings usage in line with current good style.
@bors bors closed this Jul 20, 2014
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 13, 2023
…=Veykril

String literals diagnose

Continues the work from rust-lang#15744 to add diagnosis errors to Str, ByteStr, and CStr literal kinds.

Also replaces `unescape_char` for `unescape_byte` to use the correct method for Byte literals.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants