-
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
Split requirements.txt to factor out hypothesis and prompt_toolkit #1407
Conversation
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.
Hey 👋🏻 thank you for addressing the issue.
I tried to test the changes locally but when I run python setup.py install
I get the following error:
Traceback (most recent call last):
File "/Users/glynatsi/src/AllAxelrod/Axelrod/setup.py", line 7, in <module>
with os.listdir("requirements/") as filenames:
AttributeError: __enter__
os.listdir("requirements/")
returns a list. I can tweak the file and get to it run; but why the codes work for you but not for me?
There were a couple of issues actually. I was using pip install instead of setup.py, but it seems like it should not have worked for me locally anyway (given the list vs context manager error you got). I rewrote setup.py to use pathlib and fixed that issue. Also, there's a difference in the pip command for the package on pypy and what you'll need to use locally. I updated the documentation with the correct command. I think it's possible to use setup.py directly, with a command like the following, but it didn't work for me locally, and easy_install is deprecated, so I'm sticking with pip.
|
Perfect 🚀 The Regarding
instead of |
@@ -10,6 +10,9 @@ repository:: | |||
|
|||
$ pip install axelrod | |||
|
|||
If you want to have access to the manual Human strategy for interactive play, use the following command to also install `prompt_toolkit`:: |
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.
Love this.
The failure of the CI looks like a specific check we had implemented to make sure no strategies were forgotten along the way:
I'm guessing this is something that can be addressed by including a pragma somewhere. Happy to take a look if that would be helpful. |
To address #1379, this PR defines three installation options for requirements:
Once merged and released, the different variants can be installed like so:
pip install axelrod[Human]
with the default case not requiring specification (e.g.
pip install axelrod
). I tested the variants and warnings locally in a venv but didn't add explicit tests to CI.Strategy import is updated to check for the presence of prompt_toolkit and include the Human strategy or not accordingly (and with a warning to the user that it's not available, when applicable).
I wasn't sure whether to include prompt_toolkit by default (the Human strategy is nice for live demos). We could go that route and instead have
We could define further variants (e.g. don't import any plotting functions) if desired.
Fixes #1379