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

Some feedback #4

Closed
bddap opened this issue Dec 1, 2021 · 2 comments
Closed

Some feedback #4

bddap opened this issue Dec 1, 2021 · 2 comments
Labels
enhancement New feature or request

Comments

@bddap
Copy link
Contributor

bddap commented Dec 1, 2021

Hey there, very cool library. I was looking it over and noticed a few miscellaneous things that might improve it. In no particular order:

There are a couple instances of sentinel values in the api.

  • Duration::ZERO indicates infinite ttl. This isn't what I would expect. Consider using Duration::MAX instead. (should make implementation simpler too)
  • When 0 is provided as an external cost, the provided Coster kicks in. This makes 0 a special value despite being a perfectly valid cost setting. Consider taking either an Option cost in setter methods, or using a separate setter method that doesn't take a cost parameter, and uses Coster to assign one.

It's advisable to make invalid states unrepresentable. Costs are expressed as i64, not u64. This struck me as odd. Are negative costs valid?

This call to tokio select has only one future, which doesn't make sense considering what select does (it races multiple futures).

Cargo clippy is a really handy tool for improving code. Among other things, it helps catch errors and keeps code idiomatic. I do recommend trying it out on this repo as can give really useful suggestions and teach you about rust idioms. It currently reports 49 suggestions for this codebase.

This last one might just be personal preference. Macros are immensely useful, but do have costs, like confusing rustfmt or sometimes confusing people trying to understand the code. That said, I see couple instances where use of custom macros can be reduced. This for example could be replaced with:

#[cfg(feature = "async")]
mod aync_test {

The consistent and idiomatic formatting is much appreciated by the way. Make it a lot easier to dive into the code.

Again I think this library is super cool and nicely written. If you'd like me to address any or all of these points via pr please let me know.

@bddap
Copy link
Contributor Author

bddap commented Dec 1, 2021

Oh, another suggestion: trait bounds on a struct definition can be, and usually are, un-ergonomic. David Tolnay, creator of serde, explains why.

Take a look at std::collections::HashMap for an alternative. Hashmap doesn't impose bounds on its generic types in the struct definition. Instead the bounds are expressed on on an impl block. If stretto::Cache<K, V, KH> removed the bounds on K and V (and maybe KH) it would be easier use.

@al8n
Copy link
Owner

al8n commented Dec 2, 2021

Hey there, very cool library. I was looking it over and noticed a few miscellaneous things that might improve it. In no particular order:

There are a couple instances of sentinel values in the api.

  • Duration::ZERO indicates infinite ttl. This isn't what I would expect. Consider using Duration::MAX instead. (should make implementation simpler too)
  • When 0 is provided as an external cost, the provided Coster kicks in. This makes 0 a special value despite being a perfectly valid cost setting. Consider taking either an Option cost in setter methods, or using a separate setter method that doesn't take a cost parameter, and uses Coster to assign one.

It's advisable to make invalid states unrepresentable. Costs are expressed as i64, not u64. This struck me as odd. Are negative costs valid?

This call to tokio select has only one future, which doesn't make sense considering what select does (it races multiple futures).

Cargo clippy is a really handy tool for improving code. Among other things, it helps catch errors and keeps code idiomatic. I do recommend trying it out on this repo as can give really useful suggestions and teach you about rust idioms. It currently reports 49 suggestions for this codebase.

This last one might just be personal preference. Macros are immensely useful, but do have costs, like confusing rustfmt or sometimes confusing people trying to understand the code. That said, I see couple instances where use of custom macros can be reduced. This for example could be replaced with:

#[cfg(feature = "async")]
mod aync_test {

The consistent and idiomatic formatting is much appreciated by the way. Make it a lot easier to dive into the code.

Again I think this library is super cool and nicely written. If you'd like me to address any or all of these points via pr please let me know.

Hi, Thanks for your feedback, your feedbacks are really cool and helpful to improve the project. Feel free to make a PR for your ideas! The cost is i64 because it is used for the SampledLFU.

@al8n al8n added the enhancement New feature or request label Dec 2, 2021
@al8n al8n closed this as completed in 85c551b May 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants