More nuanced handling of parent/fork in various commands#355
More nuanced handling of parent/fork in various commands#355vilmibm merged 12 commits intocli:masterfrom vilmibm:pr-parent-repo
Conversation
|
rubber ducking: There is desire for a This seemed very reasonable but now that I'm working on it I'm remembering that the concept of "no, THIS repo" is really fraught on the CLI. All we have to go on is But as soon as you have remotes like this: words like "this" and "self" lose meaning. you could say that origin is the standard for "this" but origin could just as easily be a parent repo that you initially cloned then added a remote for. all along, git sees three repos (local, parent on github.com, fork on github.com) while it's common day to day as humans to only think in terms of two repos (parent on github.com, fork on github.com). Personally, I prefer just having -R so you can be super explicit and not have to think of what "self" might mean at any given time. Given all this thinking I'm going to kick the I'm/we're open to other thoughts here but given that a big goal of |
|
having to update the API fixtures to make the tests happy again and it's taking a while. will resume doing that in the morning. |
outofambit
left a comment
There was a problem hiding this comment.
looks good overall! i think pulling the remote resolution into context was a good choice. left a suggestion to simplify one new method.
after looking at the tests, i'm also appreciating the simplicity of the -R override flag
| // additionally, look up the resolved repository name in case this | ||
| // git remote points to this repository via a redirect | ||
| (r.Network.Repositories[i] != nil && ghrepo.IsSame(r.Network.Repositories[i], repo)) { |
| hasBaseOverride := base != "" | ||
| baseOverride := ghrepo.FromFullName(base) | ||
| foundBaseOverride := false |
There was a problem hiding this comment.
i'm not that familiar with golang types, do they have optionals or nullables? it feels like baseOverride could be "string or nah", and we could eliminate hasBaseOverride or maybe the logic flow could go
if (base !== '') attempt to add base override repo to repos list
else get resolved remotes
the main reason i'm suggesting this is it took me awhile to tease apart the different pieces of control logic in this function
There was a problem hiding this comment.
there is not an optional type D: this is just kind of how golang would have us do unfortunately.
This PR:
pr createto thecontextpackagecommandpackage--repo/-Rprlist,status,viewuses the wrapper ini'm going to leave this alone for now since it has slightly different behaviorpr checkoutissuecommandscloses #251
performance note
I unscientifically benchmarked the before/after of doing this check. The time hit averaged about .2 sec on my machine/wifi. subjectively it was imperceptible and I don't think caching is urgent.
Originally based on #348