-
Notifications
You must be signed in to change notification settings - Fork 165
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 Makefile to auto setup repo for developers #699
Conversation
@tonywu315 add some notes around how to run it in |
Head branch was pushed to by a user without write access
Head branch was pushed to by a user without write access
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.
just a couple things
Co-authored-by: Taylor Turner <[email protected]>
Co-authored-by: Taylor Turner <[email protected]>
Co-authored-by: Taylor Turner <[email protected]>
Co-authored-by: Taylor Turner <[email protected]>
Makefile
Outdated
setup: requirements.txt requirements-dev.txt requirements-test.txt | ||
python -m venv venv | ||
|
||
. venv/bin/activate; \ |
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.
should these end with &&
instead of ;
?
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.
&&
returns a syntax error: unexpected end of file
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 believe this is bc the last line has \
and shouldn't end in &&
or ;
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.
@tonywu315 I would test locally this rec from @JGSweets ... I tested it one way but didn't work on my end
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.
It works for me if I remove the \
on the last line.
Makefile
Outdated
pip install -r requirements-test.txt; \ | ||
pip install -e .; \ | ||
pre-commit install; \ | ||
pre-commit run; \ |
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.
should we make a section for make test
and make formatting
?
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.
We could. I see this as just an initial implementation that improves the user experience -- one command to set their environment up, but I'll defer to @tonywu315 as the author of the PR
Co-authored-by: Taylor Turner <[email protected]>
Makefile
Outdated
pip install -r requirements-test.txt; \ | ||
pip install -e .; \ | ||
pre-commit install; \ | ||
pre-commit run; \ |
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.
last line shouldn't have a \
Makefile
Outdated
. venv/bin/activate && \ | ||
pip3 install -r requirements.txt && \ | ||
pip3 install -r requirements-dev.txt && \ | ||
pip3 install -r requirements-test.txt && \ | ||
pip3 install -e . && \ | ||
pre-commit install && \ | ||
pre-commit run |
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.
Since this is for dev, I believe we need to include the requirements-reports.txt
and requirements-ml.txt
for the install as well.
source ./venv/bin/activate | ||
``` | ||
This Makefile creates a Python virtual environment and installs all of the developer dependencies. Alternatively, follow the steps below. | ||
|
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.
It might be good to also include the other makefile components in readme
No description provided.