Default value + Callable syntax #8
Conversation
turns out you can pass blocks to lambdas! who knew? not i.
|
Changes Unknown when pulling 5f21a87 on bessey:callable-syntax into ** on exAspArk:master**. |
| @@ -75,12 +77,20 @@ def __ensure_batched | |||
| return if __executor_proxy.value_loaded?(item: @item) | |||
|
|
|||
| items = __executor_proxy.list_items | |||
| loader = ->(item, value) { __executor_proxy.load(item: item, value: value) } | |||
| loader = -> (item, value = NULL_VALUE, &block) { | |||
| if block | |||
There was a problem hiding this comment.
I had no idea you could pass a block to a lambda, that was the only reason I extracted this, so I have re-inlined it.
CHANGELOG.md
Outdated
| @@ -15,6 +15,8 @@ that you can set version constraints properly. | |||
| #### [v1.0.4](https://github.com/exAspArk/batch-loader/compare/v1.0.3...v1.0.4) | |||
|
|
|||
| * `Fixed`: Fix arity bug in `respond_to?` [#3](https://github.com/exAspArk/batch-loader/pull/3) | |||
| * `Added`: `default_value` override option. | |||
There was a problem hiding this comment.
👍
could you please move the changelog notes to the Unreleased?
| @@ -97,7 +97,7 @@ puts users # Users | |||
|
|
|||
| But the problem here is that `load_posts` now depends on the child association and knows that it has to preload data for future use. And it'll do it every time, even if it's not necessary. Can we do better? Sure! | |||
|
|
|||
| ### Basic example | |||
| ### Basic examples | |||
There was a problem hiding this comment.
Don't forget to update the Contents section as well :)
| @@ -75,12 +77,20 @@ def __ensure_batched | |||
| return if __executor_proxy.value_loaded?(item: @item) | |||
|
|
|||
| items = __executor_proxy.list_items | |||
| loader = ->(item, value) { __executor_proxy.load(item: item, value: value) } | |||
| loader = -> (item, value = NULL_VALUE, &block) { | |||
There was a problem hiding this comment.
It's not very common but there is another way to check whether the optional argument was passed or not:
-> (item, value = (value_set = true; nil), &block) {
if block
raise ... if value_set
...
else
raise ... unless value_set
...
end
}This way you can leave your argument checks (raise) without touching and preloading the value to check it's value.
There was a problem hiding this comment.
That's neat, i didn't know that was possible in that context, although I don't find it particularly legible.
I also consider (*item_and_maybe_value, &block) followed by decomposition in the block, but didn't like that it changes the arity to be incredibly permissive.
I think the existing implementation is the most readable of the 3, if not necessarily the most clever.
bessey
force-pushed
the
callable-syntax
branch
from
5f21a87 to
6b6ccb8
Compare
|
Fixed the docs comments, hope you agree on the remaining code comment. Thanks for review! |
|
Changes Unknown when pulling 6b6ccb8 on bessey:callable-syntax into ** on exAspArk:master**. |
| @@ -29,7 +30,11 @@ def load(item:, value:) | |||
| end | |||
|
|
|||
| def loaded_value(item:) | |||
| loaded[item] | |||
| if value_loaded?(item: item) | |||
There was a problem hiding this comment.
I just realized that with the changes the source code became not thread-safe :)
It's possible to have race-conditions when Thread1 read the value_loaded?(item: item) value and Thread2 loaded value after that, before Thread1 finished executing loader. Need to think how to avoid such situations, probably with some locking mechanisms (hope they won't affect performance a lot).
There was a problem hiding this comment.
Was able to reproduce the race-condition in #10. Will think what's the best way to fix it.
Per discussion in #7 / #5, I've removed the auto append functionality, and reworked the tests to make clearer why you would want to use this
#callsyntax too.