Conversation
|
@Liam-DeVoe I will keep iterating on this. The rationals generator seems a bit annoying to use, but not sure what a more ergonomic design would look like atm. |
…el-rust into echoumcp1/impl-num
DRMacIver
left a comment
DRMacIver
left a comment
There was a problem hiding this comment.
I don't love the API. If this is the best we can do because of rust limitations then so be it, but I don't think it is and I'd really rather these feel more contiguous with the main API if possible.
| @@ -1,3 +1,3 @@ | |||
| { | |||
| "nocov": 521 | |||
| "nocov": 548 | |||
There was a problem hiding this comment.
Why did the ratchet go up? I'd expect everything in this PR to be coverable, and the goal of the ratchet is to drive coverage to 100% so introducing new untested code is to be avoided wherever possible.
There was a problem hiding this comment.
I bumped it too high by accident. Misread 540 as 548. Some are just uncovered closing braces. In cbor_to_bigint I added 3 no covs under the uninteresting panic cases.
There was a problem hiding this comment.
I don't think any of them should be uncovered closing braces - the script already filters those out, and should auto-ratchet the rest down.
Fair enough RE the uninteresting panic cases. I'd like to get us covering those in general but you shouldn't have to do that in this branch.
DRMacIver
left a comment
DRMacIver
left a comment
There was a problem hiding this comment.
LGTM. I'd ideally like the as_basic feature before merging, but it's not critical, and I don't need to rereview before merging.
| } | ||
| } | ||
|
|
||
| /// Set the denominator generator. Must not produce zero. |
There was a problem hiding this comment.
I wonder if it would be better to handle the zero case.
I think there are a couple of principled things we could do here. Adding a filter would make this non-basic in cases where it should be basic, but we could add an assume or it might actually make sense to just replace zeroes with one.
| } | ||
| } | ||
|
|
||
| impl<T, NG, DG> Generator<Ratio<T>> for RationalGenerator<NG, DG> |
There was a problem hiding this comment.
It would be good for this to implement as_basic, because whenever numer_gen and denom_gen are basic, this can also be basic.
There was a problem hiding this comment.
The design of these generators accepts sub-generators: rationals(gen1, gen2); complex(gen1, gen2). Hypothesis doesn't do this, and instead accepts parameters controlling the domain of the generators: complex(min_magnitude=..., max_magnitude=...), fractions(min_value=..., maxv_value=...).
Accepting sub-generators gives Hegel substantially less control over the distribution, which we want because most users are going to pass bad generators. I think we should change to the API design of Hypothesis.
Two paths here I think. One is to add fractions() to the core and call into that. The other is to generate a denominator and generator a numerator within the range given the min/max value bounds, but we lose as_basic and of course it's two calls to the server. I'm leaning towards the former since calling into the server is really slow. |
|
I'd add fractions and complex into the protocol, @DRMacIver thoughts? |
Seems basically reasonable to me yeah. |
3 participants
Closes #91