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 Makefile to auto setup repo for developers #699

Merged
merged 15 commits into from
Oct 26, 2022

Conversation

tonywu315
Copy link
Contributor

No description provided.

@taylorfturner taylorfturner enabled auto-merge (squash) October 24, 2022 12:37
@taylorfturner taylorfturner added Documentation Improvements or additions to documentation Medium Priority Significant improvement or bug / feature reducing overall performance labels Oct 24, 2022
@taylorfturner
Copy link
Contributor

@tonywu315 add some notes around how to run it in contribution.md file so its clear exactly how it should run. Thx

auto-merge was automatically disabled October 24, 2022 13:37

Head branch was pushed to by a user without write access

Makefile Outdated Show resolved Hide resolved
@taylorfturner taylorfturner enabled auto-merge (squash) October 24, 2022 14:11
auto-merge was automatically disabled October 24, 2022 14:49

Head branch was pushed to by a user without write access

Copy link
Contributor

@taylorfturner taylorfturner left a 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

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
.github/CONTRIBUTING.md Outdated Show resolved Hide resolved
.github/CONTRIBUTING.md Outdated Show resolved Hide resolved
tonywu315 and others added 4 commits October 24, 2022 14:37
Co-authored-by: Taylor Turner <taylorfturner@gmail.com>
Co-authored-by: Taylor Turner <taylorfturner@gmail.com>
Co-authored-by: Taylor Turner <taylorfturner@gmail.com>
Co-authored-by: Taylor Turner <taylorfturner@gmail.com>
Makefile Outdated
setup: requirements.txt requirements-dev.txt requirements-test.txt
python -m venv venv

. venv/bin/activate; \
Copy link
Collaborator

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 ;?

Copy link
Contributor

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

Copy link
Collaborator

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 ;

Copy link
Contributor

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

Copy link
Contributor Author

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; \
Copy link
Collaborator

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?

Copy link
Contributor

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

Makefile Outdated Show resolved Hide resolved
Co-authored-by: Taylor Turner <taylorfturner@gmail.com>
Makefile Outdated
pip install -r requirements-test.txt; \
pip install -e .; \
pre-commit install; \
pre-commit run; \
Copy link
Collaborator

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
Comment on lines 4 to 10
. 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
Copy link
Collaborator

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.

Copy link
Collaborator

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

@JGSweets JGSweets enabled auto-merge (squash) October 26, 2022 17:03
@taylorfturner taylorfturner dismissed their stale review October 26, 2022 17:04

Dismissing my own review

@JGSweets JGSweets merged commit 3d64af0 into capitalone:main Oct 26, 2022
@tonywu315 tonywu315 deleted the makefile branch October 26, 2022 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Improvements or additions to documentation Medium Priority Significant improvement or bug / feature reducing overall performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants