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

Feature/310 add invoke tasks for development #315

Merged
merged 33 commits into from
Aug 20, 2019

Conversation

robo360
Copy link
Contributor

@robo360 robo360 commented Aug 8, 2019

In response to issue #310

@robo360 robo360 changed the base branch from master to develop August 8, 2019 20:26
@robo360 robo360 requested a review from Datamance August 8, 2019 20:27
Copy link
Contributor

@Datamance Datamance left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good start.

Make sure to run black (on your tasks file especially) and importanize - both should be in your pipenv if you're properly running things from your pipenv shell

Pipfile Outdated Show resolved Hide resolved
project/settings/defaults.py Outdated Show resolved Hide resolved
tasks.py Outdated Show resolved Hide resolved
tasks.py Outdated Show resolved Hide resolved
tasks.py Outdated Show resolved Hide resolved
tasks.py Show resolved Hide resolved
@Datamance Datamance added the Developer [Audience] Affects engineering/developers only label Aug 8, 2019
@Datamance Datamance added this to the Block 2 milestone Aug 8, 2019
@Datamance
Copy link
Contributor

I merged quite a few branches into develop, so it looks like you'll need to resolve some conflicts in the dependency files.

Thankfully, this should be pretty easy :) it's not like you need to mess around with actual code.

Copy link
Contributor

@Datamance Datamance left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Second round - good job on the first round of changes!

tasks.py Outdated Show resolved Hide resolved
tasks.py Outdated Show resolved Hide resolved
tasks.py Outdated Show resolved Hide resolved
tasks.py Outdated Show resolved Hide resolved
tasks.py Outdated Show resolved Hide resolved
tasks.py Show resolved Hide resolved
Copy link
Contributor

@Datamance Datamance left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of nitpicks, great job overall!

Pipfile Outdated Show resolved Hide resolved
Pipfile Outdated Show resolved Hide resolved
tasks.py Outdated Show resolved Hide resolved
tasks.py Outdated Show resolved Hide resolved
tasks.py Outdated Show resolved Hide resolved
tasks.py Outdated Show resolved Hide resolved
tasks.py Outdated Show resolved Hide resolved
tasks.py Outdated Show resolved Hide resolved
tasks.py Outdated Show resolved Hide resolved
tasks.py Outdated Show resolved Hide resolved
Copy link
Contributor

@Datamance Datamance left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has my stamp of approval. I have a few more nitpicks - address those, merge your changes, and we're all good to go.

Congratulations! This was a lot of great work - I'm really looking forward to seeing you and Mayowa present this stuff at the Python Boston meetup!

tasks.py Outdated

if PLATFORM == "Linux":
# Installing graphviz
"""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's ok for this to be commented out for now but let's delete this if it's not being used

tasks.py Outdated Show resolved Hide resolved
tasks.py Outdated Show resolved Hide resolved
tasks.py Show resolved Hide resolved
@robo360 robo360 merged commit 5c4bfdc into develop Aug 20, 2019
@Datamance Datamance deleted the feature/300-add-invoke-tasks-for-development branch February 18, 2020 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Developer [Audience] Affects engineering/developers only
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants