Change kicker calculation to use the correct number of cards

[Stickerpants]

We hit an edge case in this hand:

pandaeight has K5, and his best hand is one pair, 66AK7 boarderboy has QT, and his best hand is one pair, 66AQT

The King out-kicks the queen, and thus is the higher hand.

Looking at the rankHand function, I notice the kicker logic currently considers all single cards in the players hand regardless of how many cards are currently used.

Poker hands must always form 5 cards, and we only consider kickers up to that point. I've adjusted the logic to use the correct amount of kickers based on the hand category.

There would be a more elegant way to determine the kicker count by counting how many cards we've already used (i.e. 4 for quads, 3 for trips) and subtracting from 5, but that seems unnecessarily clever. There's a fixed number of hand types and we know how many kickers are required for each.

[jordems]

The updated logic looks good to me. Thanks for fixing this issue!

We'll just need to update the unit tests in rank.spec.ts, as I believe this change will break some of them. It looks like CI is no longer running on PRs @jordems?

Ah looks like we had CI runs for forks were disabled, I realized the CI work flow is actually busted and got a fix pushed up to master.

So if this is rebased and re-pushed then the ci should run and catch any issues.

[Stickerpants]

Rank spec tests are adjusted to reflect the change in the scoring system.

Just an idle thought: The rank spec tests could be rewritten to be relative between hands, rather than testing the exact score matches the calculation - since the hand score is just an implementation detail, semantically what you'd care about is that a hypothetical Hand A (444AK) is ranked higher than Hand B (444AQ). This would make them less sensitive to implementation changes, like if the scoring heuristic is ever rewritten, we just care that relative hand strengths are tested correctly.

This would also help catch bugs similar to the one I fixed today.

[Stickerpants]

Also rebased upstream for the CI fixes

[mlevi15]

Rank spec tests are adjusted to reflect the change in the scoring system.

Just an idle thought: The rank spec tests could be rewritten to be relative between hands, rather than testing the exact score matches the calculation - since the hand score is just an implementation detail, semantically what you'd care about is that a hypothetical Hand A (444AK) is ranked higher than Hand B (444AQ). This would make them less sensitive to implementation changes, like if the scoring heuristic is ever rewritten, we just care that relative hand strengths are tested correctly.

This would also help catch bugs similar to the one I fixed today.

Good idea, that would definitely be a better approach to testing this.