Teach gh cs create to use current repo as default by rneatherway · Pull Request #6596 · cli/cli

rneatherway · 2022-11-11T14:21:01Z

I followed the style of pr list here by threading through the baseRepo from the root command.

That being said, the 'cs' hierarchy of commands is implemented a little differently from the others, presumably due to
its history as a extension so I am very open to feedback on how this should be implemented.

I appreciate that the tests are failing, but please consider this PR as a suggested implementation. If it looks reasonable I will get right on the tests.

cliAutomation · 2022-11-11T14:21:17Z

Hi! Thanks for the pull request. Please ensure that this change is linked to an issue by mentioning an issue number in the description of the pull request. If this pull request would close the issue, please put the word 'Fixes' before the issue number somewhere in the pull request body. If this is a tiny change like fixing a typo, feel free to ignore this message.

cmbrose

I like the idea of this change, but have some concerns with pain points that it creates. If we can address those I think the implementation idea looks fine

cmbrose · 2022-11-29T19:22:13Z

+		currentRepo, err := a.baseRepo()
+		if err != nil {
+			return err
+		}


I tried this out and had a couple pain points:

If you're not in any repo, then the command just fails - I would have expected it to fall back on existing behavior

If you are in a repo, it seems like you can only choose that repo, or a related remote - I expected that it would use the default text, but allow you to clear it out and use any repo

Thanks for taking a look, I completely agree with you about both of these issues. I'll take a look at improving the PR in line with your suggestions.

If you're not in any repo, then the command just fails - I would have expected it to fall back on existing behavior

I've fixed this 👍

If you are in a repo, it seems like you can only choose that repo, or a related remote - I expected that it would use the default text, but allow you to clear it out and use any repo

I've experimented and this is actually a generic prompt used by all commands (e.g. try running gh pr list) the first time that you run any command in a checkout with multiple remotes. If you choose one then you can proceed.

I followed the style of `pr list` here by threading through the baseRepo from the root command.

In this case we simply shouldn't provide a default repository name.

samcoe · 2023-01-04T17:50:20Z

+		if currentRepo, err := a.baseRepo(); err == nil {
+			defaultRepo = ghrepo.FullName(currentRepo)
+		}


I see two UX issues here both stemming from using SmartBaseRepoFunc to determine the base repo:

SmartBaseRepoFunc outputs error messages to stderr which seems like something we would want to silence in this case since we are only using the resulting value if there is no error. I would have to double check but I believe all other use cases of SmartBaseRepoFunc we exit on error so that the user can act on the error message from that function.

The SmartBaseRepoFunc has cases where it will return a repo that has not actually been created on GitHub. Having that set as the default and being selected would likely cause an error, right? Having the default value cause an error seems like something we would want to avoid.

Having said that, I think number 1 is a much more glaring issue that will cause confusion.

A different approach that we can take is to use the Remotes() factory method which will return a list of git remotes, and from there use the ResolvedRemote() method and use the as the default value. The ResolvedRemote() method will return the remote that was selected with the repo set-default function. This approach would solve both issues as there is no output from either of those methods and by using the remote that the user has already selected as the default we know that it exists on GitHub. This is the approach we use in repo set-default command.

Thanks for the review, great point. I've changed the implementation as you suggest.

This avoids outputting to stderr and matches the canonical approach in `gh repo set-default`.

samcoe · 2023-01-05T17:49:40Z

+		remotes, err := a.remotes()
+		if err != nil {
+			return err
+		}


If there is an error retrieving the remotes do we actually want to error out here? Seems like we could continue executing the command without specifying a default.

Following this logic, where we don't care if there is an error, I am wondering if we should implement something like the executable interface for remotes:

type remoter interface { Remote() clicontext.Remotes }

Where implementation of the Remote() function will return an empty list of remotes if there was ever an error retrieving the git remotes.

If there is an error retrieving the remotes do we actually want to error out here? Seems like we could continue executing the command without specifying a default.

Yes, that makes sense, I've made that change.

I am wondering if we should implement something like the executable interface for remotes

I had a look at this and wasn't sure about the readability of the result. Factory already has a field Remotes and adding a method Remote with a similar signature felt awkward. Perhaps TryRemotes to keep the plural form (as it returns a list) but to make it intuitive that it cannot fail. The Try prefix is more of a .NET-ism, I haven't seen something similar used widely in Go, only Must to indicate a function panics on failure.

These are only used to suggest a default, so erroring out is a bit over the top.

samcoe

LGTM, thanks for making the requested changes!

samcoe · 2023-01-10T00:36:07Z

+		remotes, _ := a.remotes()
+		if remotes != nil {
+			defaultRemote, _ := remotes.ResolvedRemote()
+			if defaultRemote != nil {
+				defaultRepo = ghrepo.FullName(defaultRemote)
+			}
+		}


Small style nit:

Suggested change

remotes, _ := a.remotes()

if remotes != nil {

defaultRemote, _ := remotes.ResolvedRemote()

if defaultRemote != nil {

defaultRepo = ghrepo.FullName(defaultRemote)

}

}

if remotes, err := a.remotes(); err != nil {

if defaultRemote, err := remotes.ResolvedRemote(); err != nil {

defaultRepo = ghrepo.FullName(defaultRemote)

}

}

We're ignoring the errors deliberately here, so I think you meant if remotes, _ := a.remotes(); remotes != nil { right (and the same for the second line)? I've made that change.

rneatherway · 2023-01-11T17:49:24Z

LGTM, thanks for making the requested changes!

Thanks for the feedback to make this a much better change!

mislav

Thank you for your patience! I've pushed a tweak that makes it work even in freshly cloned repositories where remotes were never resolved (ResolvedRemote will always return nil)

rneatherway requested review from a team as code owners November 11, 2022 14:21

rneatherway requested review from mislav and removed request for a team November 11, 2022 14:21

cliAutomation added the external pull request originating outside of the CLI core team label Nov 11, 2022

cmbrose requested changes Nov 29, 2022

View reviewed changes

rneatherway added 3 commits December 6, 2022 17:20

Teach gh cs create to use current repo as default

f8be31d

I followed the style of `pr list` here by threading through the baseRepo from the root command.

Don't fail if we are not in a repo

ca5bc8a

In this case we simply shouldn't provide a default repository name.

Fix tests

365ca81

rneatherway force-pushed the cs-create-default-repo branch from fcae4d2 to 365ca81 Compare December 6, 2022 17:31

cmbrose approved these changes Dec 12, 2022

View reviewed changes

samcoe reviewed Jan 4, 2023

View reviewed changes

rneatherway added 2 commits January 5, 2023 15:15

Merge branch 'trunk' into cs-create-default-repo

4f94a0f

Use factory Remotes() method to obtain default repo

5e6556e

This avoids outputting to stderr and matches the canonical approach in `gh repo set-default`.

samcoe reviewed Jan 5, 2023

View reviewed changes

nitindudeja2107 mentioned this pull request Jan 6, 2023

Allow multiple account credentials #326

Open

Ignore errors loading remotes

be3fdaa

These are only used to suggest a default, so erroring out is a bit over the top.

rneatherway force-pushed the cs-create-default-repo branch from 0c5b969 to be3fdaa Compare January 6, 2023 12:16

samcoe approved these changes Jan 10, 2023

View reviewed changes

Minor code restructure

7be64e6

mislav added 2 commits February 7, 2023 20:47

Merge remote-tracking branch 'origin' into cs-create-default-repo

652d292

make it work in fresh repos too

e1c950a

mislav approved these changes Feb 7, 2023

View reviewed changes

mislav enabled auto-merge (squash) February 7, 2023 19:55

mislav merged commit 78fb909 into cli:trunk Feb 7, 2023

rneatherway deleted the cs-create-default-repo branch February 8, 2023 09:25

Uh oh!

Conversation

rneatherway commented Nov 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cliAutomation commented Nov 11, 2022

Uh oh!

cmbrose left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samcoe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rneatherway commented Jan 11, 2023

Uh oh!

mislav left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rneatherway commented Nov 11, 2022 •

edited

Loading

mislav left a comment •

edited

Loading