-
Couldn't load subscription status.
- Fork 0
refacto MemoryADT implementation for KmsEncryptionLayer #46
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good ! the few comments are just form, I think this will stop the stack overflowing
|
|
||
| pub(crate) fn findex_number_of_threads() -> Option<usize> { | ||
| std::env::var("GITHUB_ACTIONS").is_ok().then_some(1) | ||
| std::env::var("GITHUB_ACTIONS").map(|_| 1).ok() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while 100% equivalent the old syntax was more verbose and hence easier to understand for me but feel free to close this if you prefer the new one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a concurrent modification I kept through the rebase. I guess which version is more readable depends on personal taste. I for example feels the other version to be less straightforward since it involves more Rust-specific helper functions (I don't know by heart the signature of is_ok(), while map is standard).
I can revert this change if you prefer.
This is a pure refacto and I do not expect it to solve the stack overflow issue at all. |
|
all good, we can merge |
An arguably simpler version of the
MemoryADT:IntoIteratorfor an option to merge the two branches of the if inguarded_writebatch_read