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
Fix set-default interactive not showing all remotes #6969
Conversation
When running `gh repo set-default` in interactive mode only 5 remotes were showing. This change refactors the methods that fetch the info for those remotes. The methods now expect a limit to be provided. Passing `0` allows running things with no limit. Fixes cli#6854
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.
@despreston This is coming along nicely, thanks for getting started on this work. I added some code suggestions, mostly style and naming ideas.
As for tests, I don't know of any open PRs that are modifying context/context but I would say that I think the only test that is necessary to be added is one in setdefault_test.go confirming that we are resolving more than 5 remotes. What do you think?
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.
@despreston I went ahead and made the changes I suggested and also wrote up a test. I am planning on getting this into the release later today, hope you don't mind my overstep!
|
@samcoe Not at all, glad I could help. |
When running
gh repo set-defaultin interactive mode only 5 remoteswere showing. This change refactors the methods that fetch the info for
those remotes. The methods now expect a limit to be provided. Passing
0allows running things with no limit.Fixes #6854
Note to reviewers
I was unsure if it made sense to start writing tests for context/context.go. Lots going on in that file and it would suck if someone else has a WIP branch to write those tests.
The description I'm leaving for the
ResolvedRemotes.NetworkReposmethod is questionable and I'd be surprised if someone can't come up with something better.