search

release-depot / git_wrapper

A python based wrapper around GitPython
MIT License
4 stars 8 forks source link

Add git clone support #34

Closed jpichon closed 6 years ago

jguiditta commented 6 years ago

Agreed on all points. I was certainly not suggesting we move everything into a single mega-object, which I think you realize, but it is a good anti-pattern to beware of nonetheless. Maybe we can just update this with the minor .format tweak and the rest can be deferred to fight another day?

fuzzball81 commented 6 years ago

+1 to that, lets get the format fixes in place and then setup an issue to rotate the architecture to a new design.

jpichon commented 6 years ago

I agree with the fishy design issue, I think this may be why I'm finding it more difficult to name and organise things for the next patches, since it's no longer a 1-to-1 mapping to existing git functions. I also agree it would easy to fall into a "everything is a function for a single repo object" trap instead :-)

I'll update this patch with the format options, and also create an issue for the logging improvements so everywhere can be updated at once.

Is it acceptable if I also submit a PR for https://github.com/jpichon/git_wrapper/commit/77ca45 (after I update it with the format fixes, and probably just make it part of the GWClone class to sidestep some of the naming problems), or would we rather freeze till we figure out the new design after this one?

fuzzball81 commented 6 years ago

I would recommend that we get this PR landed with the format fixes. For the next set of changes, the challenge is not having to refactor a large change set to a new design, but not losing that work. It might be best to also start a PR with those changes, get them landed then focus our efforts on changing the design. We might even want to do a release with all of this new functionality then redesign with possible API breaks.

Lets see what others think before proceeding.

jguiditta commented 6 years ago

ACK to going ahead with that next PR, since the work is done. As for releasing and then doing a subsequent api change in a later release, I would say - if we need it for something else that wants to use the code, sure, otherwise I would just wait on release until this change comes in. I have a feeling we can tweak the design without it taking forever. I have a couple ideas already, though I need to go through the code a bit more to see how they fit, and if additional bits would be needed.

jpichon commented 6 years ago

Thanks for following up. I opened issue #36 based on the discussion here, looking forward to hear your & everyone's ideas! My own aren't particularly elegant so far, I'll give it some more thought and go update that other patch for now.