rafecolton / kamino

Known for its cloning technologies (really good at cloning repos from GitHub)
MIT License
1 stars 0 forks source link

Initial implementation #1

Closed rafecolton closed 10 years ago

rafecolton commented 10 years ago

Do not merge until:

jszwedko commented 10 years ago

I'm curious to see what the intent of this library is, but one thing that jumped out at me was the use of a map for options which feels a unidiomatic to me, especially since some fields are required. You may consider using named arguments for the required fields and something like what Rob Pike describes in http://commandcenter.blogspot.nl/2014/01/self-referential-functions-and-design.html?m=1 for the optional ones. Just a thought though, I haven't found what feels like a great pattern for this yet; just that a map feels a little messy.

jszwedko commented 10 years ago

Never mind, I see the intent from the diff :) was only looking at what is in master.

rafecolton commented 10 years ago

@jszwedko I agree with you, this does feel a bit messy. In particular, having the genome type be private felt odd as did having the validation inside of NewGenome() (or having a NewGenome() function at all). I think the current organization is actually a function of me writing that part of the code first. I think this can be dramatically simplified by moving the validation inside of the Clone() function. Let me see what I can whip up.

Also, the intent for this library is to be used in the docker build server :smile:

rafecolton commented 10 years ago

@jszwedko what do you think about the latest commit? Should simplify things a bit as well as making them more idiomatic (plus I'm a lot happier about it than using a map)

jszwedko commented 10 years ago

@rafecolton Figured this was for the docker build server! Definitely feels much better to me after your last commit.

rafecolton commented 10 years ago

@jszwedko :thumbsup:

I'd love to get your thoughts on this function. It's come up in a couple other contexts as well and I have yet to find the magical incantation.

rafecolton commented 10 years ago

@jszwedko @meatballhat @jtwb would you guys mind taking a look and sanity checking this? In particular, I'm looking for feedback on the updateToRef() function in clone_creator.go.

radkin commented 10 years ago

:thumbsup: