Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added strategies Desperate, Willing, Hopeless, Grim #686

Merged
merged 7 commits into from
Aug 16, 2016
Merged

Conversation

marcharper
Copy link
Member

@marcharper marcharper commented Aug 10, 2016

Adding strategies/tests Desperate, Hopeless, Willing and Grim in mutual.py

This is a rebase of PR #683 by @ranjinidas

@drvinceknight
Copy link
Member

Looks good to me, just missing 4791d13 which adds the new module to the autodoc of the strategies.

@marcharper
Copy link
Member Author

marcharper commented Aug 10, 2016

@ranjinidas is going to set the initial plays to random as well, then I think it's ready to merge!

@ranjinidas
Copy link
Member

@marcharper and @drvinceknight Can I commit to this PR? Or should I open a new one? I would have to squash all my commits again in that case.

@marcharper
Copy link
Member Author

@ranjinidas You can add to this one.

@drvinceknight
Copy link
Member

Can she? Does @ranjinidas have push access to the mutual branch? (just asking in case I'm missing something and you've done something smart @marcharper :))

@marcharper
Copy link
Member Author

If you can't push @ranjinidas, you can make a new branch off this one and open a PR from that branch to this one.

@ranjinidas
Copy link
Member

I merged local master with upstream/mutual, then ran git push -u origin upstream/mutual (clearly this was wrong. because it didn't update). How should I push upstream/mutual?

@marcharper
Copy link
Member Author

Well it's a bit of mess since I squashed these commits already. Here's what you'll need to do.

  • Rather than change the first play in first_play_test you'll need to have the players return random_choice() in their definitions. This will make first_play_test not work since it depends on the random seed. No worries on that.
  • If you want a new commit to merge easily on this branch, try the following.
git fetch origin # gets all the new commits and branches from origin, assuming that's the main repository
git checkout origin/mutual
# This makes a new branch with HEAD at the end of this branch (origin/mutual)
git branch mutual2 # you probably already have a branch named mutual
git checkout mutual2

Make any new commits on mutual2, then push mutual2 to your fork and open a pull request from rajinidas/mutual2 to origin/mutual (use "compare across forks").

Let me know if any issues come up!

If you have troubles I can git cherry-pick your new commits here.

@ranjinidas
Copy link
Member

@marcharper Thanks so much for your help! I'll try this today and let you know if there are any issues.

@marcharper
Copy link
Member Author

This is ready for review -- I grabbed @ranjinidas latest commits and added a few more tests.


def test_strategy(self):
seed(1)
self.first_play_test(C)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we also check that D is sometimes played?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And for the other two.

Suggestion: instead of a seed could just play 20 first plays and check that both C and D are in there.

@marcharper
Copy link
Member Author

Ok I'll add these in a bit.

@marcharper
Copy link
Member Author

Tests and references added; rebased for bibliography.

@drvinceknight
Copy link
Member

Looks great. Thanks @ranjinidas!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants