-
Notifications
You must be signed in to change notification settings - Fork 5.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
isolate repo view command #1409
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great!
I have mostly stylistic suggestions, but one blocker for this PR is the BaseRepo implementation. See comments
var toView ghrepo.Interface | ||
if opts.RepoArg == "" { | ||
var err error | ||
toView, err = opts.BaseRepo() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part is tricky. The invocation here is correct, but I think that for backwards compatibility, we'll need to pass a different BaseRepo
implementation to the current crop of commands that use the ResolveRemotesToRepos strategy. (Basically, all commands that are not gh api
and gh gist
.)
Right now, most of the commands that operate on the current repository use ResolveRemotesToRepos in order to figure out whether they should make requests against the current repository or its parent (if in a fork). We are considering moving away from this lookup strategy #924, but until we do, I think it's best to keep the current behavior.
Right now, the BaseRepo implementation that is passed through a Factory is the legacy base repo resolution backed by fsContext.BaseRepo
, which only consults git remotes based on their names but performs no resolution via API requests. I've specifically chosen this strategy for gh api
because I did not want any magic nor additional API requests in its implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try and massage either determineBaseRepo or a copy of it into a form usable by isolated commands
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not thrilled with what I did; mainly because the non-test factory version of ResolvedBaseRepo
is not getting exercised in tests. What do you think?
Part of #1405
Part of #1082
This PR ports the
repo view
command to the new architecture.