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

Add strategy index test to travis. #980

Merged
merged 7 commits into from
Apr 20, 2017
Merged

Add strategy index test to travis. #980

merged 7 commits into from
Apr 20, 2017

Conversation

drvinceknight
Copy link
Member

Closes #957. I am expecting this to fail on travis: two modules are not currently indexed (hmm and memorytwo). Using that as a test that this script works (once travis fails, will push the fix).

@drvinceknight
Copy link
Member Author

drvinceknight commented Apr 19, 2017

4738a96 has failed as expected (travis build here: https://travis-ci.org/Axelrod-Python/Axelrod/builds/223638310):

 0.04s$ python run_strategy_indexer.py
memorytwo not in index
The command "python run_strategy_indexer.py" exited with 1.

Just pushing the fix.

index_path="./docs/reference/all_strategies.rst",
excluded=("_strategies", "__init__", "_filters", "human")):
"""
Check if a module is contained in the index of strategies
Copy link
Member

Choose a reason for hiding this comment

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

Can we improve the docstring? Technically these aren't modules. Also let's use the standard style for parameters (just module_path is ok?) and the return value.

"""
strategies_reference = read_index(index_path)
module_name = get_module_name(module_path)
if module_name not in excluded:
Copy link
Member

Choose a reason for hiding this comment

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

could use a single if (either way is ok)


if __name__ == "__main__":

modules = glob.glob("./axelrod/strategies/*py")
Copy link
Member

Choose a reason for hiding this comment

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

"*.py" might be slightly more future proof

@drvinceknight
Copy link
Member Author

I think I've addressed your comments @marcharper. I've also added type hinting :)

@meatballs
Copy link
Member

I'm happy with this but I think an improvement might be to use the Pathlib module for file paths.

I don't think there's a problem here, but I've have had problems with file paths on windows before and Pathlib handles all that perfectly.

@drvinceknight
Copy link
Member Author

drvinceknight commented Apr 20, 2017 via email

This actually cleans up a lot of the code as well.
@drvinceknight
Copy link
Member Author

but I've have had problems with file paths on windows before and Pathlib handles all that perfectly.

Great shout on Pathlib, it cleared up a lot of the code (no real point in 2 of the 3 functions as they're just simple method calls of the Path class). I'd seen it used before but never played around with it myself: it's lovely!.

Note: I removed the type specification in the docstring. This is mainly because I noticed just before pushing that it had become a lie (I changed the type hint to be a Path and didn't change the docstring). Seems sensible to me to remove the types from the docstrings when we have type hints in place but I'm happy to throw them back in for this function if we want them.

@meatballs
Copy link
Member

Great shout on Pathlib, it cleared up a lot of the code (no real point in 2 of the 3 functions as they're just simple method calls of the Path class). I'd seen it used before but never played around with it myself: it's lovely!.

Yep. I'm a big fan!

@meatballs meatballs merged commit d9ee722 into master Apr 20, 2017
@meatballs meatballs deleted the 957 branch April 20, 2017 12:11
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.

A script to automatically detect if all strategy modules are listed in docs/reference/all_strategies.rst
3 participants