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 connector #26

Closed
wants to merge 3 commits into from
Closed

Add connector #26

wants to merge 3 commits into from

Conversation

saylorsd
Copy link
Member

@bsmithgall Forgot to remake this PR. Please feel free to take a look whenever you have the time. Note: there are no tests written yet - I plan to do that Monday - but I figured at least this way you can take look.

@bsmithgall
Copy link
Collaborator

This is a good start I think -- but I think it could be improved a little bit.

  1. I think you should either explicitly enforce having a connector object instantiated via the enforce_full_pipeline method on the pipeline object itself, or alternatively allow no connectors.
  2. If you go with the former, there is a good amount of duplication between a Connector object and an Extractor object which makes me think that maybe we are overengineering this here? Maybe instead we should just dump the Connector class and have a base-class method on the Extractor class called connect and let it be implemented by different subclasses or handled via mixin.

@bsmithgall
Copy link
Collaborator

Thinking about it a bit more, I'm starting to lean towards destroying the Connector class (sorry!) -- if you look at the existing codebase, there's just a lot of pass-between from the two classes. I think answer provides some context about what I'm thinking.

Basically, you shouldn't have the target needed to be thrown into multiple places. It should only have to be declared once and not necessarily shared among the pipeline. If it is, it feels like the concerns aren't properly separated.

@saylorsd
Copy link
Member Author

@bsmithgall That makes a lot of sense. I definitely kept getting the vibe that it wasn't making things simpler, but couldn't think of a good alternative. I'll work on implementing what you described and I'll let you know when I make any significant commits if you'd like to take a look.

@bsmithgall
Copy link
Collaborator

👍

If you want you could also take a crack at #15 and I could do some work on this, or I could take that from you as I think those are the last two big ones.

@saylorsd
Copy link
Member Author

@bsmithgall That works for me. I already stared a bit on #15, so I'll finish that up.

@bsmithgall
Copy link
Collaborator

@saylorsd I am working on an implementation similar to this that I think is a bit promising. Do you have some time this afternoon to hop on a call and talk through implementation of FTP (for #17) or HTTP (for #32)

@saylorsd
Copy link
Member Author

@bsmithgall Sounds good! I'm free until 4, so whenever works for you will probably work for me.

@bsmithgall
Copy link
Collaborator

Because this branch has failing tests & merge conflicts, I'm going to close it and open a different PR if that's alright.

@bsmithgall bsmithgall closed this Jan 27, 2016
@saylorsd saylorsd deleted the add-connector branch January 27, 2016 19:04
@bsmithgall
Copy link
Collaborator

@saylorsd take a look here

@saylorsd
Copy link
Member Author

👍 Looks good. Are you waiting for the other two know connectors to be implemented before you make the pull request?

@bsmithgall
Copy link
Collaborator

Yeah or at least one
On Jan 27, 2016 1:21 PM, "Steven Saylor" notifications@github.com wrote:

[image: 👍] Looks good. Are you waiting for the other two know
connectors to be implemented before you make the pull request?


Reply to this email directly or view it on GitHub
#26 (comment).

@saylorsd
Copy link
Member Author

Sounds good. Do you plan on/already started doing so or mind if I take a swing at it?

@bsmithgall
Copy link
Collaborator

Go for it. I'll ping you when I'm home and we can pair on the rest.
On Jan 27, 2016 1:24 PM, "Steven Saylor" notifications@github.com wrote:

Sounds good. Do you plan on/already started doing so or mind if I take a
swing at it?


Reply to this email directly or view it on GitHub
#26 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants