search

mikemccllstr / dominionstats

The code behind councilroom.com.
http://councilroom.com
11 stars 3 forks source link

Fix for Issue37 #42

Closed DLu closed 11 years ago

mikemccllstr commented 11 years ago

I've merged your changes and added tests for the PurplePileDriver problems in https://github.com/mikemccllstr/dominionstats/tree/issue_37.

I'd like to add tests for f195bf9, but I'm unsure where this crops up. Can you point me to a game or someplace where the old logic has a problem that is fixed by the new logic?

DLu commented 11 years ago

The previous logic would only tally up cards you bought/gained on your own turn, thus resulting in the problem mentioned in the original issue report. https://github.com/mikemccllstr/dominionstats/issues/37

Varsinor gained all of the Curses on their opponent's turn, which meant that data was stored in a different place than when Sayron gained all of the curses by buying them on their last turn.

mikemccllstr commented 11 years ago

Hmmm.... Changing it this way will affect other goals that use the cards_accumalated_per_player() method. Is that going to have the intended effect? For example, if I bought only money and VP, but my opponent gave me an action card, and I go on to win, shouldn't I still receive the BOMMinator achievement?

Seems like the BOM* goals, OneTrickPony, and MrGreenGenes ought to continue to use the old logic, since they explicitly mention "Buy". The pile driver goals are probably better to use the new logic, since they mention "Gained". Thoughts?

mikemccllstr commented 11 years ago

BTW, I'm in progress on a change to do this. It's not committed yet, but I wanted to let you know so we don't duplicate work.