search

WPRDC / wprdc-etl

MIT License
8 stars 3 forks source link

Add connector #26

Closed saylorsd closed 8 years ago

saylorsd commented 8 years ago

@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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

:+1:

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 commented 8 years ago

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

bsmithgall commented 8 years ago

@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 commented 8 years ago

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

bsmithgall commented 8 years ago

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

bsmithgall commented 8 years ago

@saylorsd take a look here

saylorsd commented 8 years ago

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

bsmithgall commented 8 years ago

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

[image: :+1:] 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 https://github.com/UCSUR-Pitt/wprdc-etl/pull/26#issuecomment-175808842.

saylorsd commented 8 years ago

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

bsmithgall commented 8 years ago

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 https://github.com/UCSUR-Pitt/wprdc-etl/pull/26#issuecomment-175809941.