Confirm name change before creating a repo with special characters by bchadwic · Pull Request #4562 · cli/cli

bchadwic · 2021-10-19T05:32:43Z

Submitting a draft to close #4558

This feature asks for confirmation if the name typed in for a new repo will be different from what will end up on the GH backend.

Example:
passing name as an arg

$ gh repo create C#
? Visibility Public
? Would you like to add a .gitignore? No
? Would you like to add a license? No
? Your new repository will be created as C-. Continue? No
Discarding...

interactive prompt

$ gh repo create
? Repository name C#
? Repository description 
? Visibility Public
? Your new repository will be created as C-. Continue? No
Discarding...

one liner

$ gh repo create -y --private C#
? Your new repository will be created as C-. Continue? No
Discarding...

possible bug? As a user, -y means Skip the confirmation prompt, however, why force something you didn't mean in the first place? Input needed.

mislav

Thanks for this! I have an important request of having this logic work only in interactive mode.

pkg/cmd/repo/create/create.go

mislav

Oops, I wanted to "Request Changes" instead 😅

bchadwic · 2021-10-20T01:58:30Z

@mislav

Hopefully this is a little better :)
I agree with all the points made above, so the changes made reflect that.

I'm not sure how I feel about this though:

$ gh repo create C#
? Visibility Public
? Would you like to add a .gitignore? No
? Would you like to add a license? No
? This will create the "C-" repository on GitHub. Continue? Yes
? This will add an "origin" git remote to your local repository. Continue? No
Discarding...

$ gh repo create C
? Visibility Public
? Would you like to add a .gitignore? No
? Would you like to add a license? No
? This will add an "origin" git remote to your local repository. Continue? No
Discarding...

Could the two confirmations at the end be combined? I suppose the first confirmation only shows if the name will be normalized, so not a big deal but it still seems kind of painful to confirm twice.

mislav

Almost there!

mislav · 2021-10-20T11:21:40Z

pkg/cmd/repo/create/create.go

-			alreadyDashed = true
-		}
+func normalizeRepoName(name string) string {
+	re := regexp.MustCompile(`[^a-zA-Z0-9-._]+`)


Since - has special meaning inside [...] in regular expressions, a literal - inside of [] should always be at the very end, just before the closing ].

Instead of a-zA-Z0-9_ you could use \w which is equivalent.

Since this regular expression is static, you can declare it on the package level instead of in the function: var invalidRepoNameCharacters = regexp.MustCompile(...)

mislav · 2021-10-20T11:25:09Z

pkg/cmd/repo/create/create.go

-		}
+func normalizeRepoName(name string) string {
+	re := regexp.MustCompile(`[^a-zA-Z0-9-._]+`)
+	if normalizedRepoName := strings.TrimSuffix(re.ReplaceAllString(name, "-"), ".git"); normalizedRepoName != "" {


There is no need for this if, I think. Just return the result of TrimSuffix and regexp replacement.

mislav · 2021-10-20T11:33:39Z

pkg/cmd/repo/create/create_test.go

-		Name       string
-		LocalName  string
-		RemoteName string
+		Name           string


It doesn't look like the Name field is used in the tests, so you can remove it

mislav · 2021-10-20T12:09:02Z

pkg/cmd/repo/create/create.go

 	if inLocalRepo {
+		if normalizedRepoName != repoName {
+			var confirmNormalizedName bool
+			err := survey.AskOne(&survey.Confirm{


Now that I see how this adds an additional prompt to the inLocalRepo case:

This will create the "OWNER/REPO" repository on GitHub. Continue? Yes This will add an "origin" git remote to your local repository. Continue?

I wonder whether we could just fold this into a single prompt:

This will create "OWNER/REPO" on GitHub and add it as the "origin" git remote. Continue?

What do you think?

Agreed, I think the confirm prompt should contain the new repo name regardless of inLocalRepo or if the name was normalized.

I removed the inLocalRepo parameter from confirmSubmit for this reason, was that safe to assume?

mislav · 2021-10-21T10:14:50Z

pkg/cmd/repo/create/create.go

-		return normalizedRepoName
-	}
-	return "-"
+	return strings.TrimSuffix(regexp.MustCompile(`[^\w.-]+`).ReplaceAllString(name, "-"), ".git")


This looks great now. I love how much simpler it has become 😄 Just had a thought: could you add a comment to this function explaining that this intends to match the logic by which GitHub the platform also normalize repo names?

@mislav

Thanks! I've been sharpening up on my regex skills because of this repo haha

I have a humble suggestion. Since I've been creating repos with the new feature, I tend to cross check and confirm that what I typed in is the same as what will be normalized in the confirm prompt every time.

Something that I like about the website, is that I can quickly know without cross checking, if my name will be changed thanks to the nifty color system.

Green -> what I typed in is the same as what will appear on the server
Yellow -> something in my repo name will be normalized
Red -> empty string (currently if .git is inputted, gh repo create will try to make a repo with an empty string as the name)

Could we do a colorizing system (Green / Yellow) for just the name so users can just confirm using the color? If this crowds the code or seems unnecessary I understand. Also if the name is an empty string, should we still call the API knowing that it will fail?

For example, without color this might be a pain. For accessibility we could also adapt the text the web site uses. Maybe I'm getting ahead of myself on this one though.

The color switching for this isn't a bad idea, but I think it would be overkill for this feature right now. Let's hold off for now and see whether we feel it's actually necessary in the future.

pkg/cmd/repo/create/create.go

bchadwic · 2021-10-27T18:21:12Z

Sorry for the hold up! I think I have it close to being ready. I added the conditional statement back in for inLocalRepo

This is what it looks like now in use

$ gh repo create example!
? This will create the "example-" repository on GitHub. Continue? No

# after creating a local .git

$ gh repo create
? Repository name (example!)
? This will create "example-" on GitHub and add it as the "origin" git remote. Continue? No

We plan a larger refactoring around this aspect of repo create functionality, but until then, it would be best to keep the original behavior.

Gotcha! I'll keep an eye on this one :)

mislav · 2021-11-02T15:04:56Z

pkg/cmd/repo/create/create.go

+	normalizedRepoName := normalizeRepoName(repoName)
 	if inLocalRepo {
-		promptString = `This will add an "origin" git remote to your local repository. Continue?`
+		promptString = fmt.Sprintf(`This will create "%s" on GitHub and add it as the "origin" git remote. Continue?`, normalizedRepoName)


Let's use the OWNER/REPO format in this prompt rather than just REPO since the else branch in this code also uses the OWNER/REPO format for the repository in the prompt. Rule of thumb is: repositories should always be displayed in OWNER/REPO format to reduce ambiguity.

mislav · 2021-11-02T15:06:10Z

pkg/cmd/repo/create/create.go

+
+// normalizeRepoName takes in the repo name the user inputted and normalizes it using the same logic as GitHub (GitHub.com/new)
+func normalizeRepoName(repoName string) string {
+	return strings.TrimSuffix(regexp.MustCompile(`[^\w.-]+`).ReplaceAllString(repoName, "-"), ".git")


Underscore _ is also an allowed character

https://regex101.com/r/Sl6WtS/1
https://pkg.go.dev/regexp/syntax

I think \w includes _? I added _ to the regex just to be sure, but I supplied a test golang regex link above as well as the docs.

bchadwic · 2021-11-03T05:41:12Z

pkg/cmd/repo/create/create.go

+	targetRepo := normalizeRepoName(repoName)
+	if repoOwner != "" {
+		targetRepo = fmt.Sprintf("%s/%s", repoOwner, targetRepo)
+	}


I moved the targetRepo assignment outside of the if inLocalRepo statement. Is this what you meant above about HOST/REPO?

mislav

Thank you; this looks good! I've resolved conflicts with the base branch and I love how minimal we brought the diff down to 😄

bchadwic and others added 2 commits October 18, 2021 20:57

moving to laptop dev

aca719c

finished out tests

d9e1c5f

bchadwic requested a review from a team as a code owner October 19, 2021 05:32

bchadwic requested review from mislav and removed request for a team October 19, 2021 05:32

fixing lint check

1c6e31d

mislav approved these changes Oct 19, 2021

View reviewed changes

pkg/cmd/repo/create/create.go Outdated Show resolved Hide resolved

pkg/cmd/repo/create/create.go Outdated Show resolved Hide resolved

pkg/cmd/repo/create/create.go Outdated Show resolved Hide resolved

mislav suggested changes Oct 19, 2021

View reviewed changes

samcoe assigned mislav Oct 19, 2021

bchadwic added 3 commits October 19, 2021 18:02

updating per changes requested

8a70a10

merging modified branch to pr

b860325

oops

cce2e28

mislav suggested changes Oct 20, 2021

View reviewed changes

mislav changed the title ~~closes #4558, produce a confirmation before creating a repo with special characters~~ Confirm name change before creating a repo with special characters Oct 20, 2021

bchadwic added 2 commits October 20, 2021 11:21

collapsed if statement, reduced regex, refactored regex test structs

b3db2ec

removed unnecessary parameter

c682797

mislav suggested changes Oct 21, 2021

View reviewed changes

adding doc to func

3c7a0b1

mislav suggested changes Oct 22, 2021

View reviewed changes

pkg/cmd/repo/create/create.go Outdated Show resolved Hide resolved

added in inLocalRepo logic statement again

5c883a1

samcoe added the hacktoberfest-accepted label Oct 27, 2021

mislav suggested changes Nov 2, 2021

View reviewed changes

fixing regex, and targetRepo

8f2c782

bchadwic commented Nov 3, 2021

View reviewed changes

Merge remote-tracking branch 'origin' into special-characters

8f1ca44

mislav approved these changes Nov 18, 2021

View reviewed changes

mislav enabled auto-merge (squash) November 18, 2021 13:21

mislav merged commit 6cc7712 into cli:trunk Nov 18, 2021

VictorBatta mentioned this pull request Dec 4, 2021

GitHub CLI 2.3.0 #4855

Closed

jenkins-infra-bot mentioned this pull request Dec 20, 2021

[updatecli] Bump ghcli version jenkins-infra/docker-builder#26

Merged

Conversation

bchadwic commented Oct 19, 2021

Uh oh!

mislav left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mislav left a comment

Choose a reason for hiding this comment

Uh oh!

bchadwic commented Oct 20, 2021

Uh oh!

mislav 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!

bchadwic Oct 20, 2021 • 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!

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!

Uh oh!

bchadwic commented Oct 27, 2021

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!

mislav left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bchadwic Oct 20, 2021 •

edited

Loading