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

Test cleanup issue 1287 #1308

Merged
merged 17 commits into from
Apr 6, 2020
Merged

Test cleanup issue 1287 #1308

merged 17 commits into from
Apr 6, 2020

Conversation

sudarshan-parvatikar
Copy link
Contributor

I have refactored the code according to Issue:1287. I also re-ordered import statements to make them consistent across all files and ran tests. The tests passed and I thinks this should close Issue:1287.

@drvinceknight
Copy link
Member

There seems to be a merge conflict, can you rebase this on to master?

@sudarshan-parvatikar
Copy link
Contributor Author

Sorry about the merge conflicts, this was the first time I was exposed to merge conflicts. I rebased to master and fixed merge conflicts. The tests also passed on my end.

Copy link
Member

@gaffney2010 gaffney2010 left a comment

Choose a reason for hiding this comment

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

This is a really nice clean up.

It looks like there is a few lines missed.

It also looks like your editor changed the import order of unrelated imports. I don't think we've been consistent about import order. But some of these changes add a lot of new lines, and I think we probably don't want to do that.


import axelrod
import axelrod as axl
from axelrod import AntiCycler, Cycler, EvolvableCycler
Copy link
Member

Choose a reason for hiding this comment

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

Delete this line and update references throughout file.

import random
import unittest

import axelrod as axl
from axelrod import EvolvablePlayer, seed
Copy link
Member

Choose a reason for hiding this comment

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

Delete this line, and update.

import warnings

import axelrod
import axelrod as axl
from axelrod import Game
Copy link
Member

Choose a reason for hiding this comment

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

Delete line and update

@@ -1,22 +1,27 @@
import unittest
Copy link
Member

Choose a reason for hiding this comment

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

Should there be space between each of these imports?

import numpy as np

import axelrod as axl
from axelrod import DefaultGame, Player, LimitedHistory
Copy link
Member

Choose a reason for hiding this comment

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

Delete and update

import matplotlib.pyplot as plt

import axelrod as axl
from axelrod import ApproximateMoranProcess, MoranProcess, Pdf
Copy link
Member

Choose a reason for hiding this comment

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

Delete and update

from numpy import mean, nanmedian, std

import axelrod as axl
import axelrod.interaction_utils as iu
Copy link
Member

Choose a reason for hiding this comment

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

Looking at the issue. It looks like it was decide that only axelrod would be imported like this, and that you'd use axl.interaction_utils elsewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's right.

@@ -1,19 +1,29 @@
"""Tests for the main tournament class."""

import unittest
Copy link
Member

Choose a reason for hiding this comment

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

spacing

from collections import Counter

import axelrod
import axelrod as axl
import axelrod.interaction_utils as iu
Copy link
Member

Choose a reason for hiding this comment

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

Delete this line. Ditto from comment on other file.

Copy link
Contributor Author

@sudarshan-parvatikar sudarshan-parvatikar Apr 5, 2020

Choose a reason for hiding this comment

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

Ditto from comment on other file.

Can you tell me which comment that you're talking about?

Copy link
Member

Choose a reason for hiding this comment

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

import axelrod.interaction_utils as iu

It looks like it was decided to just import axelrod, and to replace iu with axelrod.interaction_utils.

import random
import unittest

import axelrod as axl
import axelrod.strategy_transformers as st
Copy link
Member

Choose a reason for hiding this comment

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

Delete this line. It looks like that is what was decided on another file.

@sudarshan-parvatikar
Copy link
Contributor Author

This is a really nice clean up.

It looks like there is a few lines missed.

It also looks like your editor changed the import order of unrelated imports. I don't think we've been consistent about import order. But some of these changes add a lot of new lines, and I think we probably don't want to do that.

Hey @gaffney2010! . Thank you for your helpful comments. I reordered the import statements so that they'd be consistent. However, you're right about adding new lines, so I'll update them as per your reviews. Furthermore, is it ok if the import statements be as in PEP 8 import rules, as explained by this answer on SO. This may possibly make the import statements more consistent in the future.

@gaffney2010
Copy link
Member

Furthermore, is it ok if the import statements be as in PEP 8 import rules, as explained by this answer on SO.

Yes.

@sudarshan-parvatikar
Copy link
Contributor Author

I have updated the code as per the recent review. Imports are grouped according to the PEP-8 convention, and new lines between the import statements are removed

@gaffney2010
Copy link
Member

This looks good aside from the three places where you have:
import axelrod.xxx as yyy

I think it was decided in the issue to only import axelrod.

@sudarshan-parvatikar
Copy link
Contributor Author

This looks good aside from the three places where you have:
import axelrod.xxx as yyy

I think it was decided in the issue to only import axelrod.

Yes, I now understand what you meant to say. I'll fix it asap :)

@drvinceknight
Copy link
Member

Thanks @sudarshan-parvatikar all looks good to me 👍

@drvinceknight drvinceknight merged commit d4735cb into Axelrod-Python:master Apr 6, 2020
@sudarshan-parvatikar
Copy link
Contributor Author

Thanks all for being so patient and helpful. 😄

@sudarshan-parvatikar sudarshan-parvatikar deleted the test-cleanup-issue-1287 branch April 6, 2020 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants