belliottsmith commented on code in PR #57:
URL: https://github.com/apache/cassandra-accord/pull/57#discussion_r1302180335
##########
accord-core/src/test/java/accord/burn/random/FrequentLargeRange.java:
##########
@@ -18,59 +18,121 @@
package accord.burn.random;
-import accord.utils.Invariants;
+import accord.utils.Gen;
+import accord.utils.Gen.LongGen;
+import accord.utils.Gens;
import accord.utils.RandomSource;
-public class FrequentLargeRange implements RandomLong
+import java.time.Duration;
+import java.util.concurrent.TimeUnit;
+
+public class FrequentLargeRange implements LongGen
{
- private final RandomLong small, large;
- private final double ratio;
- private final int steps;
- private final double lower, upper;
- private int run = -1;
- private long smallCount = 0, largeCount = 0;
-
- public FrequentLargeRange(RandomLong small, RandomLong large, double ratio)
+ private final LongGen small, large;
+ private final Gen<Boolean> runs;
+
+ public FrequentLargeRange(LongGen small, LongGen large, double ratio)
{
- Invariants.checkArgument(ratio > 0 && ratio <= 1);
this.small = small;
this.large = large;
- this.ratio = ratio;
- this.steps = (int) (1 / ratio);
- this.lower = ratio * .8;
- this.upper = ratio * 1.2;
+ this.runs = Gens.bools().biasedRepeatingRuns(ratio);
Review Comment:
Perhaps we are talking past each other? Now we no longer utilise the `Gen`
API within the top-level declarations of these classes, I am suggesting we move
forwards. Your examples are comparing this patch in its current form with the
patch in its form when this disagreement arose, which is to say where
`Gen.LongGen` etc was peppered around the implementation of the aforementioned
classes.
https://github.com/apache/cassandra-accord/pull/57/commits/49da37997ab4b1406c0069c9f387ab025d8990bb
This is mixing styles in my book. In fact, there remain examples I would
prefer we removed, e.g.
https://github.com/apache/cassandra-accord/pull/57/commits/49da37997ab4b1406c0069c9f387ab025d8990bb#diff-d7eab055034d6123ac548c3da83300a6ab4aa050e057d3d2cf03b059ab6df502R197
- this is very different to my style of authorship, which would directly
compose primitive actions on `RandomSource` since that is entirely sufficient
here. I haven't asked for that, though; just to isolate the usages to as close
to the instantiation as possible, and not confuse the API/top level of the
classes.
My style of code does not use `Gen` at all, and introducing it - as
demonstrated above - leads to a very different style of code being written.
This is totally fine when authoring your own classes, but it's less well
received by authors of their own classes that they're still maintaining, in
which consistency of style will be harmed. I don't think this is the first time
we've discussed our conflicting approaches to test authorship, so this
shouldn't be a huge surprise? If your style and its components begin to weave
their way amongst my style of authorship, then suddenly I have to have opinions
about how your API is structured, it's inescapable (but that's its own can of
worms, neither of us have time for right now).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
