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

Convert `Storage::entries`'s `max_size` argument to `Option<u64>` #98

Closed
bkchr opened this Issue Jul 26, 2018 · 14 comments

Comments

Projects
None yet
5 participants
@bkchr
Copy link

bkchr commented Jul 26, 2018

Currently the value is just a u64 with u64::MAX defined as NO_LIMIT.
This no limit is not mentioned in the documentation. Something like the following enum would suite better:

enum Size {
    Fixed(u64),
    NoLimit,
}; 

I could provide a PR, if wanted.

Edit (brson): per discussion, let's change this to Option<u64> with a doc-comment explaining that None means "no limit". The Storage trait is in src/storage.rs.

@siddontang

This comment has been minimized.

Copy link
Member

siddontang commented Jul 27, 2018

Maybe adding a comment is better.

/cc @Hoverbear

@Hoverbear

This comment has been minimized.

Copy link
Member

Hoverbear commented Jul 27, 2018

@bkchr Would adding a comment about this limit be satisfactory? Since NoLimit == Fixed(u64::MAX)

@bkchr

This comment has been minimized.

Copy link
Author

bkchr commented Jul 27, 2018

Yeah I know that NoLimit == Fixed(u64::MAX) after looking into the code. I'm unsure. I think a comment would be a good start if you don't want to add an extra enum. However, adding an enum would be the cleaner solution in my point of view.

@Hoverbear

This comment has been minimized.

Copy link
Member

Hoverbear commented Sep 7, 2018

Perhaps Option<u64> is appropriate. A None value correctly communicates that The max size is none.

@bkchr

This comment has been minimized.

Copy link
Author

bkchr commented Sep 8, 2018

Yeah, an option would also fit here ;)

@Hoverbear

This comment has been minimized.

Copy link
Member

Hoverbear commented Sep 10, 2018

@bkchr Would you still be interested in making a PR, or is this open for others to tackle?

@bkchr

This comment has been minimized.

Copy link
Author

bkchr commented Sep 10, 2018

Open for others :)

@brson brson changed the title `Storage::entries` `max_size` should be an `Enum` Convert `Storage::entries` `max_size` to `Option<u64>` Feb 11, 2019

@brson brson changed the title Convert `Storage::entries` `max_size` to `Option<u64>` Convert `Storage::entries`'s `max_size` argument to `Option<u64>` Feb 11, 2019

@estk

This comment has been minimized.

Copy link

estk commented Feb 13, 2019

Id like to work on this!

@estk

This comment has been minimized.

Copy link

estk commented Feb 13, 2019

dang, too slow :/

@pwoolcoc

This comment has been minimized.

Copy link
Contributor

pwoolcoc commented Feb 13, 2019

sorry @estk, I should have commented when I started working on it :-/

if you are so inclined, I'd love some code review of my PR!

@estk

This comment has been minimized.

Copy link

estk commented Feb 13, 2019

Sure thing @pwoolcoc, you're too quick on the draw :)

@Hoverbear

This comment has been minimized.

Copy link
Member

Hoverbear commented Feb 13, 2019

@estk Aw, thanks so much! Do you think you might be interested in this one instead? #70

@estk

This comment has been minimized.

Copy link

estk commented Feb 14, 2019

@Hoverbear haha, you really gotta be quick here! Thanks for pointing that one out, but it looks like I'm too late again. I'll be away for the weekend but I'll swing back next week and try to pick up an issue. Thanks for the hospitality @Hoverbear

pwoolcoc pushed a commit to pwoolcoc/raft-rs that referenced this issue Feb 14, 2019

@Hoverbear

This comment has been minimized.

Copy link
Member

Hoverbear commented Feb 14, 2019

🤦‍♀️ Ha, I'm sorry. :)

pwoolcoc pushed a commit to pwoolcoc/raft-rs that referenced this issue Feb 19, 2019

@brson brson closed this in #183 Feb 26, 2019

brson added a commit that referenced this issue Feb 26, 2019

Converts the `max_size` argument in `Storage::entries` to an `Option<…
…u64>` (#183)

* Closes #98

* rustfmt pass

* Treat `Some(NO_LIMIT)` the same as `None`

* Address some review comments

* Change signatures of both these `entries` methods to allow compatibility

Changing the `max_size` parameter to an Option<u64> will break existing
callers, so this commit changes max_size to be `Into<Option<u64>>` so
existing callers will not need to change.

It will, however, break implementers of the `Storage` trait, but I
thought that was less common than calling the methods

* address code review comments

* Change `slice` to use `impl Into<Option<u64>>`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.