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

Rename try_fill or fill_bytes? #27

Closed
dhardy opened this issue Oct 30, 2017 · 6 comments
Closed

Rename try_fill or fill_bytes? #27

dhardy opened this issue Oct 30, 2017 · 6 comments

Comments

@dhardy
Copy link
Owner

dhardy commented Oct 30, 2017

I'm not going to break out regexxer yet because I'm not convinced either way yet, but we should probably resolve this.

@dhardy dhardy added this to the rand-core RFC milestone Oct 31, 2017
@pitdicker
Copy link

An other thought: for try_fill it may make sense to return the number of filled bytes. In case of an error, maybe there is still part of the result ready.

fn try_fill(&mut self, dest: &mut [u8]) -> Result<usize, Error>;

If we go this route, fill_bytes should return the number of filled bytes too. If we were to make this change, it seems ok to me to also change the name to fill.

fn fill(&mut self, dest: &mut [u8]) -> usize;

@newpavlov
Copy link

I personally prefer fill more, as bytes part should be clear from the type signature. Also try_fill_bytes feels a bit clunky. But I can live with the old name as well.

@dhardy
Copy link
Owner Author

dhardy commented Oct 31, 2017

But @pitdicker try_fill should fill the buffer unless it fails, so it only makes sense to return a usize on error — unless this becomes like typical read functions which may succeed with less data than requested, but this significantly complicates usability.

@pitdicker
Copy link

Hm yes I messed up. If you return Result<usize, Error> it will give either a usize or an Error, not both. Then there is no argument in that for renaming.

There is no other simple way to say a slice is only partially filled, is there?

I don't have a preference for fill vs try_fill. Well, I like fill more, but do not al that much given the breakage.

It is not completely clear to me how much breakage we have until now. Is it only a little, or so much one more change does not really make it worse?

@dhardy
Copy link
Owner Author

dhardy commented Nov 3, 2017

I don't know; I guess there are a fair few users of rand by now, but no idea how many use fill_bytes.

dhardy added a commit that referenced this issue Dec 21, 2017
This is heavily inspired by #27 by @GrahamDennis but simpler trait and
maintains backwards compatibility with deprecations.
dhardy added a commit that referenced this issue Jan 3, 2018
This is heavily inspired by #27 by @GrahamDennis but simpler trait and
maintains backwards compatibility with deprecations.
@dhardy
Copy link
Owner Author

dhardy commented Jan 12, 2018

We merged try_fill_bytes: rust-random#225

@dhardy dhardy closed this as completed Jan 12, 2018
dhardy added a commit that referenced this issue Jan 29, 2018
This is heavily inspired by #27 by @GrahamDennis but simpler trait and
maintains backwards compatibility with deprecations.
dhardy added a commit that referenced this issue Jan 29, 2018
This is heavily inspired by #27 by @GrahamDennis but simpler trait and
maintains backwards compatibility with deprecations.
dhardy added a commit that referenced this issue Jan 29, 2018
This is heavily inspired by #27 by @GrahamDennis but simpler trait and
maintains backwards compatibility with deprecations.
dhardy added a commit that referenced this issue Jan 29, 2018
This is heavily inspired by #27 by @GrahamDennis but simpler trait and
maintains backwards compatibility with deprecations.
dhardy added a commit that referenced this issue Feb 10, 2018
This is heavily inspired by #27 by @GrahamDennis but simpler trait and
maintains backwards compatibility with deprecations.
dhardy added a commit that referenced this issue Feb 15, 2018
This is heavily inspired by #27 by @GrahamDennis but simpler trait and
maintains backwards compatibility with deprecations.
dhardy added a commit that referenced this issue Feb 19, 2018
This is heavily inspired by #27 by @GrahamDennis but simpler trait and
maintains backwards compatibility with deprecations.
pitdicker pushed a commit to pitdicker/rand that referenced this issue Apr 4, 2018
This is heavily inspired by dhardy#27 by @GrahamDennis but simpler trait and
maintains backwards compatibility with deprecations.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants