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

Rng renames: gen_ → random_ #1505

Merged
merged 9 commits into from
Oct 16, 2024
Merged

Rng renames: gen_ → random_ #1505

merged 9 commits into from
Oct 16, 2024

Conversation

dhardy
Copy link
Member

@dhardy dhardy commented Oct 9, 2024

  • Added a CHANGELOG.md entry

Summary

This appears to be the conclusion of #1503:

rng.random()
rng.random_iter()
rng.random_range(..len)
rng.random_bool(0.2)
rng.random_ratio(2, 3)
rng.sample(Open01)
rng.sample_iter(Open01)
rng.fill(&mut buf)

Opinion

My personal feeling is that this is a poor choice. We recently renamed rand::distributions to rand::distr because the latter was easier to type and consistent with rand_distr (see #1381). This achieves some level of consistency (excepting that random_bool describes its output while other random* methods describe their input), but is not concise.

@dhardy dhardy marked this pull request as ready for review October 11, 2024 09:01
@dhardy dhardy requested a review from newpavlov October 11, 2024 09:02

/// Alias for [`Rng::random_range`].
#[inline]
#[deprecated(since = "0.9.0", note = "Renamed to `random_range`")]
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it's better to do a deprecating backport and release in the v0.8 branch and remove these methods completely in v0.9?

Copy link
Member 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 we should add a #[deprecated] notice in a patch release because (a) it may break some builds (https://doc.rust-lang.org/cargo/reference/semver.html#new-lints) and (b) it effectively forces users who don't want to see a warning to update their code just because they updated to the next patch version.

Since the next release is a breaking release, we could just not add these #[deprecated] fn wrappers, but they potentially ease migration and I don't see much harm in leaving them in until 1.0 (or 0.10).

/// ```
///
/// [`Bernoulli`]: distr::Bernoulli
#[inline]
#[track_caller]
fn gen_ratio(&mut self, numerator: u32, denominator: u32) -> bool {
fn random_ratio(&mut self, numerator: u32, denominator: u32) -> bool {
Copy link
Member

@newpavlov newpavlov Oct 11, 2024

Choose a reason for hiding this comment

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

I think this name could be misleading. It sounds as if it generates a "random ratio". But I don't have a better proposal than much longer random_bool_with_ratio or the generic argument proposal, so I guess it's fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is code like this confusing? I don't think it's particularly confusing:

if rng.random_ratio(1, 100) {
   do_something();
}

An alternative is that we just remove the method and tell people to use Bernoulli::from_ratio(n, d).unwrap().sample(&mut rng), but I don't see a real reason to remove this little utility fn.

@vks
Copy link
Collaborator

vks commented Oct 11, 2024

I don't really like the new name. I would prefer something like sample_* or uniform_*, but I guess this is too technical.

I agree that random_ratio is misleading, but so is gen_ratio. I think bool_from_ratio or bool_from_prob would also work.

@dhardy dhardy merged commit 8225d94 into rust-random:master Oct 16, 2024
15 checks passed
@dhardy dhardy deleted the rng-renames branch October 16, 2024 15:12
benjamin-lieser pushed a commit to benjamin-lieser/rand that referenced this pull request Oct 24, 2024
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.

3 participants