-
Notifications
You must be signed in to change notification settings - Fork 271
Add docstrings #1377
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 docstrings #1377
Conversation
Added docstrings to files which had not merged. Simplified chained comparisons in SelfSteem Changed set(["game"]) to {"game"} to reduce function call Deleted duplicate TestIdentityDualTransformer class
.isort.cfg
Outdated
@@ -4,4 +4,4 @@ multi_line_output = 3 | |||
include_trailing_comma = True | |||
force_grid_wrap = 0 | |||
combine_as_imports = True | |||
line_length = 80 | |||
line_length = 88 |
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.
This line here is causing a number of other things to break.
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.
python -m isort --recursive axelrod/.
and python -m black -l 80
.
seem to do conflicting changes.
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.
They shouldn't based on the setting in https://github.com/Axelrod-Python/Axelrod/blob/dev/.isort.cfg but it could be that the specific version of isort
you're using isn't the one we have setup in the CI: https://github.com/Axelrod-Python/Axelrod/blob/dev/.github/workflows/config.yml#L52
(We should clarify that somewhere.)
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.
Sorry this PR is proving to be a pain for you, if there's something specific I can do to assist please ask. We'll have to have a think afterwards how we can avoid the friction you're experiencing. It hasn't really happened in the past but we've made a number of small tweaks to our CI recently which might be the cause.
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.
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.
Taking a look now :)
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.
I've opened https://github.com/caddycarine/Axelrod/pull/1 on your fork. This is a PR to this branch so once you merge that it should update this PR and should get the CI to pass.
The main problem seemed to be caddycarine@bbe9a72
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.
Your solution works 😁!
Thanks.
Modified .isort.cfg to resolve merge issues.
1. Modified max.size in strategy_lists because len(axl.short_run_time_strategies) < len(axl.strategies) in property.py 2. Added classifiers_list method, in test_filtering.py, and used it in test_boolean_filtering.
Fix isort config
@@ -1001,20 +1001,6 @@ class TestFlippedDualTransformer(TestPlayer): | |||
} | |||
|
|||
|
|||
class TestIdentityDualTransformer(TestPlayer): |
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.
Why remove this?
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.
Repetition; an identical class was defined earlier..
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.
Great 👍 Thank you.
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.
This looks good to me. 👍
Add docstrings to strategy methods so they don't display the default placeholder docstring and simplify chained comparisons in SelfSteem class