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

Consider providing Rng methods as inherent methods #862

Closed
crlf0710 opened this issue Aug 9, 2019 · 6 comments
Closed

Consider providing Rng methods as inherent methods #862

crlf0710 opened this issue Aug 9, 2019 · 6 comments
Labels
E-question Participation: opinions wanted

Comments

@crlf0710
Copy link

crlf0710 commented Aug 9, 2019

Rng is almost always used, so maybe consider providing its methods to at least thread_rng.
Maybe using https://github.com/dtolnay/inherent ,

@dhardy
Copy link
Member

dhardy commented Aug 9, 2019

Traits are a core part of the Rust language. Is adding stuff like this really in the best interests of users, or is it simply causing confusion (I don't need to import Rng so why do I need to import SeedableRng)?

The other angle on this is that we have been asked quite a few times to minimise dependencies.

@dhardy dhardy added the E-question Participation: opinions wanted label Aug 9, 2019
@newpavlov
Copy link
Member

newpavlov commented Aug 9, 2019

Note that it will cause duplication of methods in documentation, and if we'll add doc(hidden), then I think it will only cause confusion. Personally I believe inherent trait implementations should be a language feature: rust-lang/rfcs#2375

@burdges
Copy link
Contributor

burdges commented Aug 9, 2019

Already addressed by use rand::prelude::*;

@matklad
Copy link
Contributor

matklad commented Aug 15, 2019

Case point: std::io::Stdin has read_line inherent method, which is a convenience over BufRead::read_line(stdin().lock()).

I think a good design for maximizing usability is Python's random: it re-exports methods on default global rng as module-level functions.

rand sort-of does this, but it only exports random, which proxies to Rng::gen, while other functions are not proxied. Additionally, the function is renamed, which seems less than ideal.

With uniform paths, I think re-exporting all the methods as is will be nice:

rand::gen
rand::gen_range
rand::sample

@dhardy
Copy link
Member

dhardy commented Aug 15, 2019

We could do that. We have been more leaning towards suggesting users cache the rng handle at least within their current function.

If we want to expand this to choose I guess we'd have to pick the more general implementation, for iterators. Performance isn't much different so long as size hints are available.

A drawback is the arbitrary choice of which functions to make available this way: choose_weighted? partial_shuffle? gen_ratio?

Personally I don't think requiring a single import (prelude::* or Rng) is too bad.

@dhardy
Copy link
Member

dhardy commented Aug 28, 2019

Proposed resolution: reject. This adds to API surface for only minor convenience, and the rand::randomrand::gen suggestion would be a breaking change.

In pursuit of open design, I will leave this topic open for discussion for some more time. Evidence-based arguments are preferred, but this is mostly an opinion issue.

@dhardy dhardy closed this as completed Sep 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-question Participation: opinions wanted
Projects
None yet
Development

No branches or pull requests

5 participants