Closed dcr8898 closed 1 month ago
@flash-gordon, I have implemented the strategy that I described above. It works, but it breaks some specs for PredicateInferrer
.
I can see from the PredicateInferrer
specs that your established method for calling nullary predicates is to wrap them in an array, and that you combine them with unary predicates by chaining another constrained
method. These specs could be changed to use the proposed method signature instead of an array, but the existence of these specs implies that the proposed solution is a breaking change (even if the broken behavior is undocumented outside of the specs).
The proposed solution also breaks the spec for constraints defined on optional
types (constrained_spec.rb:226). This is fixable by double-splatting the options
argument passed to .constrained
in sum.rb:89. This doesn't seem to break any other tests.
I don't want to go any further unless I know this is a desired path. I think the proposed solution is more intuitive than using an array and multiple .constrained
methods (or passing dummy arguments), but if it breaks existing code then it would have to be introduced appropriately.
You can see the proposed change with tests here.
I just noticed that Builder
also implements a constrained
method. I presume this would have to be changed in the same way for consistency.
Before moving further I'd like to describe the assumed changes in the API.
In the new version, all these will be acceptable:
type.constrained(:odd) # new variant
type.constrained(:odd, :even) # new variant
type.constrained(:odd, gt: 200, lt: 300) # new variant
type.constrained([:odd, :even]) # worked before, continue to support
type.constrained([:odd, :even], gt: 200, lt: 300) # new variant, supported as a side-effect
type.constrained # error, as before
type.constrained([]) # worked before, continue to support
type.constrained({}) # worked before, continue to support
It's a minor issue but the new API will allocate slightly more objects because of *
and **
.
@solnic any arguments against this change? It's backwards-compatible it seems
Yes this is a good addition. Types should be memoized anyway (in typical scenarios at least) so I wouldn't worry about increased object allocs.
@flash-gordon Thank you for providing a formal specification.
These work with the current proposed implementation:
type.constrained(:odd) # new variant
type.constrained(:odd, :even) # new variant
type.constrained(:odd, gt: 200, lt: 300) # new variant
type.constrained # error, as before
These do not work with the currently proposed change, but could be implemented by adding .flatten
to the processing of the positional arguments.
type.constrained([:odd, :even]) # worked before, continue to support
type.constrained([:odd, :even], gt: 200, lt: 300) # new variant, supported as a side-effect
type.constrained([]) # worked before, continue to support
The following use cases, the last of which was not mentioned above, will only work if we add an explicit Hash check to the positional arguments.
type.constrained({}) # worked before, continue to support
type.constrained({gt: 200, lt: 300}) # worked before, will no longer work!
.flatten
and add new tests for the various use cases?predicate_inferrer_spec
?Builder#constrained
?Thank you!
I have submitted a PR (#474) to resolve this. Includes tests and yardocs. Some issues remain. See the PR for details.
@dcr8898 yeah, thanks, I'll take a look soon
That's cool. Take your time. I just thought it would be easier for you to look at code instead of dealing with my questions. 😁
Describe the bug
The documentation states that "Under the hood
dry-types
uses dry-logic and all of its predicates are supported." However, there is no obvious way to use nullary predicates (predicates that don't take an argument--that is, arity = 0), likeodd?
.I previously raised this issue with the community.
To Reproduce
Expected behavior
There should be a conventional way to pass nullary predicates.
Workarounds
After reviewing the
Rule
class method in/lib/dry/types/constraints.rb
, I see that it expects to receive only keyword arguments that it can treat as a Hash andmap
over. This implied a workaround for using nullary predicates by including a dummy value.@flash-gordon pointed out that you can achieve the same workaround by passing an array of one or more nullary predicates.
However, this workaround does not allow users to combine nullary predicates with unary predicates (that take one argument), as can be done by explicitly passing dummy values.
Offer
I am willing to submit a pull request to correct this issue, but I don't want to do so without a conversation on implementation.
It seems to me that one way to handle this is to require nullary predicates to be grouped first, before the unary predicates that are passed as keyword arguments. This would allow us to glom the nullary predicates into an array of positional arguments, map this to a Hash with
nil
dummy values, and merge with the keyword argument Hash before processing as currently implemented.If that sounds good, I'm happy to code it up. In addition, I'm willing to update the docsite with more discussion and examples of using
dry-logic
predicates as constraints indry-types
. (Partially addressing #390)Thank You
Thank you!