Closed ctm closed 4 years ago
This may be a matter of me deriving Ord
, but implementing PartialOrd
, in lib/commands/src/evaluation/badugi.rs
. I did that in one of the other evaluators and it also resulted in a mystery bug.
Looks like this issue may be even more widespread:
bash-3.2[player-tourneys-86]$ rg 'derive.*Ord' | egrep -v 'Ord.*PartialOrd|PartialOrd.*Ord'
rg 'derive.*Ord' | egrep -v 'Ord.*PartialOrd|PartialOrd.*Ord'
lib/commands/src/evaluation.rs:#[derive(Clone, Debug, Deserialize, Eq, Ord, PartialEq, Serialize)]
lib/commands/src/evaluation/kansas_city.rs:#[derive(Clone, Debug, Deserialize, Eq, Ord, PartialEq, Serialize)]
lib/commands/src/evaluation/badugi.rs:#[derive(Clone, Debug, Deserialize, Eq, Ord, PartialEq, Serialize)]
lib/commands/src/evaluation/london.rs:#[derive(Clone, Debug, Deserialize, Eq, Ord, PartialEq, Serialize)]
I'll go through and fix all four of those tomorrow, although I'll write a test case for chuck's hand and make sure the test fails before I go through and implement those other four Ords
.
I wonder if anyone has thought about adding a clippy test for this.
I'm super glad I wrote the test, because the bug wasn't what I thought it was, but I am going to go ahead and refactor so both Ord
and PartialOrd
are implemented in the four cases above, because when that matters, it results in a subtle bug; it just doesn't happen to matter in the code as it's used today.
Here was the bug:
diff --git a/lib/commands/src/evaluation/badugi.rs b/lib/commands/src/evaluation/badugi.rs
index bd22f55..7993cd9 100644
--- a/lib/commands/src/evaluation/badugi.rs
+++ b/lib/commands/src/evaluation/badugi.rs
@@ -59,7 +59,7 @@ impl Badugi {
) -> Self {
let mut v = Vec::new();
- for rank in Rank::ACE_LOW..Rank::ACE_HIGH {
+ for rank in Rank::ACE_LOW..=Rank::ACE_HIGH {
let mut needed = true;
for suit in [&mut clubs, &mut diamonds, &mut hearts, &mut spades].iter_mut() {
if needed && (**suit & 1) == 1 {
I'll do the grunt Ord work and then deploy.
Fixed and Deployed.
My guess is that when I wrote the badugi evaluator I wrote it figuring there would only be four cards and taking that into consideration (and maybe even documenting it) but that when I added badacey and badugi I then forgot that I had that dependency.
It should be trivial to reproduce and fairly easy to fix.