-
Notifications
You must be signed in to change notification settings - Fork 263
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
Conversation
4738a96 has failed as expected (travis build here: https://travis-ci.org/Axelrod-Python/Axelrod/builds/223638310):
Just pushing the fix. |
run_strategy_indexer.py
Outdated
index_path="./docs/reference/all_strategies.rst", | ||
excluded=("_strategies", "__init__", "_filters", "human")): | ||
""" | ||
Check if a module is contained in the index of strategies |
There was a problem hiding this comment.
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.
run_strategy_indexer.py
Outdated
""" | ||
strategies_reference = read_index(index_path) | ||
module_name = get_module_name(module_path) | ||
if module_name not in excluded: |
There was a problem hiding this comment.
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)
run_strategy_indexer.py
Outdated
|
||
if __name__ == "__main__": | ||
|
||
modules = glob.glob("./axelrod/strategies/*py") |
There was a problem hiding this comment.
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
I think I've addressed your comments @marcharper. I've also added type hinting :) |
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. |
That's a good shout. I'll do that and push today 👍
…On Thu, 20 Apr 2017, 08:20 Owen Campbell, ***@***.***> wrote:
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.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#980 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACCGWgPFUEipTyorDVLn58_7Y_ZNfONKks5rxwchgaJpZM4NB8PY>
.
|
This actually cleans up a lot of the code as well.
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 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 |
Yep. I'm a big fan! |
Closes #957. I am expecting this to fail on travis: two modules are not currently indexed (
hmm
andmemorytwo
). Using that as a test that this script works (once travis fails, will push the fix).