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

921 #922

Merged
merged 4 commits into from
Mar 22, 2017
Merged

921 #922

merged 4 commits into from
Mar 22, 2017

Conversation

Chadys
Copy link
Member

@Chadys Chadys commented Mar 20, 2017

#921
I took more time than I though to correct the tests, since TestHardGoByMajority uses TestGoByMajority. Tell me if there is a problem with how I changed things.
And I also corrected the type hint of memory_depth which indicated float even though, as the docstring says on line 37 :

Parameters
        ----------
        memory_depth, int >= 0

(default value float('inf') must have confused the contributor)

Copy link
Member

@drvinceknight drvinceknight left a comment

Choose a reason for hiding this comment

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

Looks good to me. Nice cleanup! 👍

opponent = axelrod.Cooperator()
self.assertEqual('C', player.strategy(opponent))
def test_soft(self):
pass
Copy link
Member

Choose a reason for hiding this comment

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

Why is this here? Does it need a test to be added?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it's just to override the test from TestGoByMajority, that checks the soft argument works correctly. Since HardGoByMajority doesn't have a soft argument anymore, that test isn't needed and was causing an exception if I didn't override it to remove it. I thought about not making TestHardGoByMajority inherite from TestGoByMajority at all, but there are still enough common points between the two for it to be meaningful.

Copy link
Member Author

Choose a reason for hiding this comment

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

I could have used delattr instead.

opponent = axelrod.Cooperator()
self.assertEqual('C', player.strategy(opponent))
def test_soft(self):
pass
Copy link
Member

Choose a reason for hiding this comment

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

👍 Could you add a comment to explain the need for line 78? (I had to ask this again to keep github's review process happy).

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, will do tonight !

@Chadys
Copy link
Member Author

Chadys commented Mar 21, 2017

So, maybe I got a little over-enthusiastic here, but I modified more things than planned. Instead of adding a comment, I finally concluded that, since TestHardGoByMajority proceeded to less tests than TestGoByMajority, it would be more logical to inverse the genealogy. So that's what I did, making TestHardGoByMajority much clearer by not having any function dealing with the soft parameter and making only TestGoByMajority implements them. I also re-factorized test_strategy a little, to avoid code repetition. And finally I added a test that was missing (testing that GoByMajority cooperates when opponent cooperated more often than defected, as only the 'equality' or 'more defect' cases were tested). But if I overstepped my bounds please tell me, and I'll just add the comment instead.

@meatballs
Copy link
Member

But if I overstepped my bounds please tell me

I don't think we have any to be overstepped!

I'll have a proper look at this in the morning.

@marcharper
Copy link
Member

Looks good to me!

@drvinceknight
Copy link
Member

👍

@meatballs meatballs merged commit ca62919 into Axelrod-Python:master Mar 22, 2017
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.

None yet

4 participants