From 0b8c964a416b23d0918bcc34fa2f25c0b529ad81 Mon Sep 17 00:00:00 2001 From: Cristian Dominguez Date: Thu, 17 Sep 2020 23:48:25 -0300 Subject: [PATCH 1/6] Fix gist ID extraction from url --- pkg/cmd/gist/view/view.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/gist/view/view.go b/pkg/cmd/gist/view/view.go index fecb69adb..5ca209171 100644 --- a/pkg/cmd/gist/view/view.go +++ b/pkg/cmd/gist/view/view.go @@ -74,7 +74,7 @@ func viewRun(opts *ViewOptions) error { u, err := url.Parse(opts.Selector) if err == nil { if strings.HasPrefix(u.Path, "/") { - gistID = u.Path[1:] + gistID = strings.Split(u.Path, "/")[2] } } From 2af136cc780c32c68ae13911fb7c25cb3eb2edf3 Mon Sep 17 00:00:00 2001 From: Cristian Dominguez Date: Sat, 19 Sep 2020 18:48:00 -0300 Subject: [PATCH 2/6] Return error if it's an invalid URL --- pkg/cmd/gist/view/view.go | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/pkg/cmd/gist/view/view.go b/pkg/cmd/gist/view/view.go index 5ca209171..919b0d090 100644 --- a/pkg/cmd/gist/view/view.go +++ b/pkg/cmd/gist/view/view.go @@ -71,10 +71,16 @@ func viewRun(opts *ViewOptions) error { return utils.OpenInBrowser(gistURL) } - u, err := url.Parse(opts.Selector) - if err == nil { - if strings.HasPrefix(u.Path, "/") { - gistID = strings.Split(u.Path, "/")[2] + if strings.Contains(gistID, "/") { + u, err := url.Parse(opts.Selector) + if err == nil && strings.HasPrefix(u.Path, "/") { + split := strings.Split(u.Path, "/") + + if len(split) > 2 && split[2] != "" { + gistID = strings.Split(u.Path, "/")[2] + } else { + return fmt.Errorf("Invalid gist URL %s", u) + } } } From ef9b75e1f6b1dd1d734939ae56d646ec27fc2e73 Mon Sep 17 00:00:00 2001 From: Cristian Dominguez Date: Sat, 19 Sep 2020 18:50:07 -0300 Subject: [PATCH 3/6] Add tests for gist URL check --- pkg/cmd/gist/view/view_test.go | 34 ++++++++++++++++++++++++++++------ 1 file changed, 28 insertions(+), 6 deletions(-) diff --git a/pkg/cmd/gist/view/view_test.go b/pkg/cmd/gist/view/view_test.go index 0ddc33181..c50541a4f 100644 --- a/pkg/cmd/gist/view/view_test.go +++ b/pkg/cmd/gist/view/view_test.go @@ -83,11 +83,12 @@ func TestNewCmdView(t *testing.T) { func Test_viewRun(t *testing.T) { tests := []struct { - name string - opts *ViewOptions - wantOut string - gist *shared.Gist - wantErr bool + name string + opts *ViewOptions + wantOut string + wantStderr string + gist *shared.Gist + wantErr bool }{ { name: "no such gist", @@ -177,6 +178,22 @@ func Test_viewRun(t *testing.T) { }, wantOut: "some files\ncicada.txt\n\nbwhiizzzbwhuiiizzzz\n\nfoo.md\n\n- foo\n\n", }, + { + name: "url", + opts: &ViewOptions{ + Selector: "https://gist.github.com/octocat/1234", + }, + wantErr: true, + wantStderr: "HTTP 404 (https://api.github.com/gists/1234)", + }, + { + name: "invalid url", + opts: &ViewOptions{ + Selector: "https://gist.github.com/octocat", + }, + wantErr: true, + wantStderr: "Invalid gist URL https://gist.github.com/octocat", + }, } for _, tt := range tests { @@ -200,12 +217,17 @@ func Test_viewRun(t *testing.T) { io.SetStdoutTTY(true) tt.opts.IO = io - tt.opts.Selector = "1234" + if tt.opts.Selector == "" { + tt.opts.Selector = "1234" + } t.Run(tt.name, func(t *testing.T) { err := viewRun(tt.opts) if tt.wantErr { assert.Error(t, err) + if tt.wantStderr != "" { + assert.EqualError(t, err, tt.wantStderr) + } return } assert.NoError(t, err) From d685d666b2a0b7ee3481b145e875d3c2fdf99264 Mon Sep 17 00:00:00 2001 From: Cristian Dominguez Date: Mon, 21 Sep 2020 12:11:08 -0300 Subject: [PATCH 4/6] Set gist ID by default on all tests --- pkg/cmd/gist/view/view_test.go | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/pkg/cmd/gist/view/view_test.go b/pkg/cmd/gist/view/view_test.go index c50541a4f..185a2b979 100644 --- a/pkg/cmd/gist/view/view_test.go +++ b/pkg/cmd/gist/view/view_test.go @@ -91,11 +91,17 @@ func Test_viewRun(t *testing.T) { wantErr bool }{ { - name: "no such gist", + name: "no such gist", + opts: &ViewOptions{ + Selector: "1234", + }, wantErr: true, }, { name: "one file", + opts: &ViewOptions{ + Selector: "1234", + }, gist: &shared.Gist{ Files: map[string]*shared.GistFile{ "cicada.txt": { @@ -109,6 +115,7 @@ func Test_viewRun(t *testing.T) { { name: "filename selected", opts: &ViewOptions{ + Selector: "1234", Filename: "cicada.txt", }, gist: &shared.Gist{ @@ -127,6 +134,9 @@ func Test_viewRun(t *testing.T) { }, { name: "multiple files, no description", + opts: &ViewOptions{ + Selector: "1234", + }, gist: &shared.Gist{ Files: map[string]*shared.GistFile{ "cicada.txt": { @@ -143,6 +153,9 @@ func Test_viewRun(t *testing.T) { }, { name: "multiple files, description", + opts: &ViewOptions{ + Selector: "1234", + }, gist: &shared.Gist{ Description: "some files", Files: map[string]*shared.GistFile{ @@ -161,7 +174,8 @@ func Test_viewRun(t *testing.T) { { name: "raw", opts: &ViewOptions{ - Raw: true, + Selector: "1234", + Raw: true, }, gist: &shared.Gist{ Description: "some files", @@ -217,10 +231,6 @@ func Test_viewRun(t *testing.T) { io.SetStdoutTTY(true) tt.opts.IO = io - if tt.opts.Selector == "" { - tt.opts.Selector = "1234" - } - t.Run(tt.name, func(t *testing.T) { err := viewRun(tt.opts) if tt.wantErr { From 55a8f3d8caaa24995bb06f10aca751ad4d3c062c Mon Sep 17 00:00:00 2001 From: Cristian Dominguez Date: Mon, 21 Sep 2020 21:28:04 -0300 Subject: [PATCH 5/6] Make gh gist view and edit commands reuse logic for id extraction --- pkg/cmd/gist/edit/edit.go | 10 +++++----- pkg/cmd/gist/shared/shared.go | 19 +++++++++++++++++++ pkg/cmd/gist/view/view.go | 14 ++++---------- 3 files changed, 28 insertions(+), 15 deletions(-) diff --git a/pkg/cmd/gist/edit/edit.go b/pkg/cmd/gist/edit/edit.go index 554e9363d..3f03cf854 100644 --- a/pkg/cmd/gist/edit/edit.go +++ b/pkg/cmd/gist/edit/edit.go @@ -6,7 +6,6 @@ import ( "errors" "fmt" "net/http" - "net/url" "sort" "strings" @@ -69,11 +68,12 @@ func NewCmdEdit(f *cmdutil.Factory, runF func(*EditOptions) error) *cobra.Comman func editRun(opts *EditOptions) error { gistID := opts.Selector - u, err := url.Parse(opts.Selector) - if err == nil { - if strings.HasPrefix(u.Path, "/") { - gistID = u.Path[1:] + if strings.Contains(gistID, "/") { + id, err := shared.GistIDFromURL(gistID) + if err != nil { + return err } + gistID = id } client, err := opts.HttpClient() diff --git a/pkg/cmd/gist/shared/shared.go b/pkg/cmd/gist/shared/shared.go index 95d8bae31..f285621ab 100644 --- a/pkg/cmd/gist/shared/shared.go +++ b/pkg/cmd/gist/shared/shared.go @@ -3,6 +3,8 @@ package shared import ( "fmt" "net/http" + "net/url" + "strings" "time" "github.com/cli/cli/api" @@ -37,3 +39,20 @@ func GetGist(client *http.Client, hostname, gistID string) (*Gist, error) { return &gist, nil } + +func GistIDFromURL(gistURL string) (string, error) { + u, err := url.Parse(gistURL) + if err == nil && strings.HasPrefix(u.Path, "/") { + split := strings.Split(u.Path, "/") + + if len(split) > 2 { + return split[2], nil + } + + if len(split) == 2 && split[1] != "" { + return split[1], nil + } + } + + return "", fmt.Errorf("Invalid gist URL %s", u) +} diff --git a/pkg/cmd/gist/view/view.go b/pkg/cmd/gist/view/view.go index 919b0d090..aae6b93ba 100644 --- a/pkg/cmd/gist/view/view.go +++ b/pkg/cmd/gist/view/view.go @@ -3,7 +3,6 @@ package view import ( "fmt" "net/http" - "net/url" "sort" "strings" @@ -72,16 +71,11 @@ func viewRun(opts *ViewOptions) error { } if strings.Contains(gistID, "/") { - u, err := url.Parse(opts.Selector) - if err == nil && strings.HasPrefix(u.Path, "/") { - split := strings.Split(u.Path, "/") - - if len(split) > 2 && split[2] != "" { - gistID = strings.Split(u.Path, "/")[2] - } else { - return fmt.Errorf("Invalid gist URL %s", u) - } + id, err := shared.GistIDFromURL(gistID) + if err != nil { + return err } + gistID = id } client, err := opts.HttpClient() From a7a90b3a4b5ea3549e0182379118d3a36d1cea84 Mon Sep 17 00:00:00 2001 From: Cristian Dominguez Date: Mon, 21 Sep 2020 21:35:07 -0300 Subject: [PATCH 6/6] Move gist URL tests to its own file --- pkg/cmd/gist/shared/shared_test.go | 52 ++++++++++++++++++++++++++++++ pkg/cmd/gist/view/view_test.go | 30 +++-------------- 2 files changed, 57 insertions(+), 25 deletions(-) create mode 100644 pkg/cmd/gist/shared/shared_test.go diff --git a/pkg/cmd/gist/shared/shared_test.go b/pkg/cmd/gist/shared/shared_test.go new file mode 100644 index 000000000..0f80db690 --- /dev/null +++ b/pkg/cmd/gist/shared/shared_test.go @@ -0,0 +1,52 @@ +package shared + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func Test_GetGistIDFromURL(t *testing.T) { + tests := []struct { + name string + url string + want string + wantErr bool + }{ + { + name: "url", + url: "https://gist.github.com/1234", + want: "1234", + }, + { + name: "url with username", + url: "https://gist.github.com/octocat/1234", + want: "1234", + }, + { + name: "url, specific file", + url: "https://gist.github.com/1234#file-test-md", + want: "1234", + }, + { + name: "invalid url", + url: "https://gist.github.com", + wantErr: true, + want: "Invalid gist URL https://gist.github.com", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + id, err := GistIDFromURL(tt.url) + if tt.wantErr { + assert.Error(t, err) + assert.EqualError(t, err, tt.want) + return + } + assert.NoError(t, err) + + assert.Equal(t, tt.want, id) + }) + } +} diff --git a/pkg/cmd/gist/view/view_test.go b/pkg/cmd/gist/view/view_test.go index 185a2b979..0ddf55f49 100644 --- a/pkg/cmd/gist/view/view_test.go +++ b/pkg/cmd/gist/view/view_test.go @@ -83,12 +83,11 @@ func TestNewCmdView(t *testing.T) { func Test_viewRun(t *testing.T) { tests := []struct { - name string - opts *ViewOptions - wantOut string - wantStderr string - gist *shared.Gist - wantErr bool + name string + opts *ViewOptions + wantOut string + gist *shared.Gist + wantErr bool }{ { name: "no such gist", @@ -192,22 +191,6 @@ func Test_viewRun(t *testing.T) { }, wantOut: "some files\ncicada.txt\n\nbwhiizzzbwhuiiizzzz\n\nfoo.md\n\n- foo\n\n", }, - { - name: "url", - opts: &ViewOptions{ - Selector: "https://gist.github.com/octocat/1234", - }, - wantErr: true, - wantStderr: "HTTP 404 (https://api.github.com/gists/1234)", - }, - { - name: "invalid url", - opts: &ViewOptions{ - Selector: "https://gist.github.com/octocat", - }, - wantErr: true, - wantStderr: "Invalid gist URL https://gist.github.com/octocat", - }, } for _, tt := range tests { @@ -235,9 +218,6 @@ func Test_viewRun(t *testing.T) { err := viewRun(tt.opts) if tt.wantErr { assert.Error(t, err) - if tt.wantStderr != "" { - assert.EqualError(t, err, tt.wantStderr) - } return } assert.NoError(t, err)