sourcegraph / go-vcs

manipulate and inspect VCS repositories in Go
https://sourcegraph.com/sourcegraph/go-vcs
Other
79 stars 20 forks source link

Add UpdateResult to UpdateEverything method. #80

Closed dmitshur closed 8 years ago

dmitshur commented 9 years ago

This is an early draft of a sample implementation for proposal #79. It's very easy to refactor and change it.

Resolves #79.

slimsag commented 9 years ago

@shurcooL everything you have said makes sense. Sorry for my confusion! This looks good to me. If you feel confident about the change, feel free to merge. If not, feel free to ping another for additional review here.

dmitshur commented 9 years ago

@shurcooL everything you have said makes sense. Sorry for my confusion! This looks good to me.

Great, and no problem!

If you feel confident about the change, feel free to merge. If not, feel free to ping another for additional review here.

I'm kinda wearing an open-source contributor hat with this PR, and if possible, I'd rather stick to that for the rest of the process. Just learning about what the experience would be like. If it's not possible or takes too long that way, then I'll switch to my project owner hat and do the rest of the work needed to get this merged (and update our dependencies to new API, etc.). :)

dmitshur commented 9 years ago

I've addressed some review comments with the last 2 commits, PTAL.

dmitshur commented 8 years ago

Friendly ping.

slimsag commented 8 years ago

Sorry for the delays here. I've given this a second more extensive review just now, as it seems I'll be the only reviewer.

Please address adding TODO prefixes to the lack-of-implementation comments, after which I'll feel confident in merging this change.

dmitshur commented 8 years ago

After discussion in #79, there's been a change done in f63f96a to the interface:

@@ -23,8 +23,10 @@ type HTTPSConfig struct {
 type RemoteUpdater interface {
    // UpdateEverything updates all branches, tags, etc., to match the
    // default remote repository. The implementation is VCS-dependent.
-   // If supported by the implementation, results of update will be returned.
-   UpdateEverything(RemoteOpts) (UpdateResult, error)
+   //
+   // If supported by the implementation, parsed results of update will be returned,
+   // otherwise it'll be nil.
+   UpdateEverything(RemoteOpts) (*UpdateResult, error)
 }

 // UpdateResult is the result of parsing output of the remote update operation.

Needs a final review and this can be ready to merge.

beyang commented 8 years ago

LGTM

dmitshur commented 8 years ago

Great, I'll merge this soon when I have a little more bandwidth to update the dependents.

Unless someone else is blocking on this, then we can merge sooner.

slimsag commented 8 years ago

I'll be merging this as part of https://src.sourcegraph.com/sourcegraph/.changes/366

dmitshur commented 8 years ago

Woohoo, thanks!