Closed oleander closed 3 years ago
~Ignore Ruby 3.0 for now. It's related to keywords arguments. I'll investigate further later this week.~
@solnic My self-review is complete. Let me know if you want me to make any changes :)
Thanks! I need to take a closer look. I started wondering if maybe we could somehow reuse existing rule compiler for this DSL. I gave it a quick shot but it wasn't immediately obvious how to solve various challenges like how operations should be handled etc. Anyhow, I'll get back to this during this week! Stay tuned :)
@solnic I just want to clarify that the builder only map calls like eql?
to Rule::Predicate.new(Predicates[:eql?])
and check keys: [...] { ... }
to Operations::Check.new(..., ...)
using method_missing
and respond_to_missing?
. It doesn't do any calculations itself. So there shouldn't be any duplication of logic between the two implementations.
@solnic Did you find any time to play around with the builder?
This implementation requires the caller to specify an operations keyword argument.
I.e
check keys: [:key1, :key2] do
# ...
end
One of the perks of going thru the AST was the lack of these.
I.e
check [:key1, :key2] do
# ...
end
@solnic Do you think I should alter the implementation to make keyword arguments optional?
@solnic Do you think I should alter the implementation to make keyword arguments optional?
I think keeping it simple is better, at least for now.
I’ve simplified the builder by no longer relying on method_missing
. It’s also renamed to Builder
from Build
to match Evaluator
.
It looks to be performing well
Dry::Logic::Builder.call do
key?(:user) & key(name: %i[user age]) { gt?(18) }
end
vs
user_present = Dry::Logic::Rule::Predicate.new(Dry::Logic::Predicates[:key?]).curry(:user)
min_age = Dry::Logic::Rule::Predicate.new(Dry::Logic::Predicates[:gt?]).curry(18)
has_min_age = Dry::Logic::Operations::Key.new(min_age, name: %i[user age])
user_present & has_min_age
λ bundle exec ruby benchmarks/builder.rb
Warming up --------------------------------------
builder 3.546k i/100ms
regular 4.089k i/100ms
Calculating -------------------------------------
builder 36.567k (± 4.7%) i/s - 184.392k in 5.054061s
regular 39.574k (± 5.1%) i/s - 200.361k in 5.077076s
Comparison:
regular: 39573.7 i/s
builder: 36567.4 i/s - same-ish: difference falls within error
@solnic yeah, it's good
@oleander thank you for working on this man ❤️ I'll release this soon and publish your docsite additions too! 🎉
Awesome, thanks!
Let me double check the docs PR before it's merged. Just gonna make sure it looks okay as I haven't looked at it for a few months. I'll do that on Monday.
@oleander I merged it in before I saw your comment. This is published under "master" version so it's fine if it's still a WIP, because it is a WIP 🙂 I just sent you an invite to this repo so feel free to push any doc improvements directly to master, unless you're not sure if something makes sense and you need some feedback, then a PR would be great.
@solnic Thats great, thanks! I just took a quick look at the docs. It looks decent but needs some small semantic changes to the markdown and language. I'll push these directly to master on Monday.
I also think the language for some of the operations/predicates (like check) could be improved for clarity. For these I'll open a PR to make sure I understood their use case.
The later should be an ongoing process until it's released under 2.0.
What we have now is a really good foundation and will help tremendously getting started with dry/logic.
What we have now is a really good foundation and will help tremendously getting started with dry/logic.
Totally!
See #79.