From c843a4fa13a206db8331b52f93305c6a19b955ab Mon Sep 17 00:00:00 2001 From: Sam Coe Date: Fri, 13 Nov 2020 11:16:54 +0300 Subject: [PATCH 1/6] Add issue comment viewing --- api/queries_issue.go | 57 ++- api/reaction_groups.go | 52 +++ api/reaction_groups_test.go | 55 +++ pkg/cmd/issue/shared/lookup.go | 51 +-- .../view/fixtures/issueView_preview.json | 8 +- .../issueView_previewClosedState.json | 2 +- .../issueView_previewFullComments.json | 383 ++++++++++++++++++ .../issueView_previewSingleComment.json | 147 +++++++ .../issueView_previewThreeComments.json | 265 ++++++++++++ .../issueView_previewWithEmptyBody.json | 2 +- .../issueView_previewWithMetadata.json | 60 ++- pkg/cmd/issue/view/view.go | 138 ++++++- pkg/cmd/issue/view/view_test.go | 210 +++++++++- pkg/iostreams/color.go | 24 +- utils/utils.go | 16 + utils/utils_test.go | 27 +- 16 files changed, 1408 insertions(+), 89 deletions(-) create mode 100644 api/reaction_groups.go create mode 100644 api/reaction_groups_test.go create mode 100644 pkg/cmd/issue/view/fixtures/issueView_previewFullComments.json create mode 100644 pkg/cmd/issue/view/fixtures/issueView_previewSingleComment.json create mode 100644 pkg/cmd/issue/view/fixtures/issueView_previewThreeComments.json diff --git a/api/queries_issue.go b/api/queries_issue.go index 08e0cf1d9..1aa83ad89 100644 --- a/api/queries_issue.go +++ b/api/queries_issue.go @@ -33,12 +33,8 @@ type Issue struct { Body string CreatedAt time.Time UpdatedAt time.Time - Comments struct { - TotalCount int - } - Author struct { - Login string - } + Comments IssueComments + Author Author Assignees struct { Nodes []struct { Login string @@ -65,12 +61,31 @@ type Issue struct { Milestone struct { Title string } + ReactionGroups ReactionGroups } type IssuesDisabledError struct { error } +type IssueComments struct { + Nodes []IssueComment + TotalCount int +} + +type IssueComment struct { + Author Author + AuthorAssociation string + Body string + CreatedAt time.Time + IncludesCreatedEdit bool + ReactionGroups ReactionGroups +} + +type Author struct { + Login string +} + const fragments = ` fragment issue on Issue { number @@ -320,7 +335,7 @@ loop: return &res, nil } -func IssueByNumber(client *Client, repo ghrepo.Interface, number int) (*Issue, error) { +func IssueByNumber(client *Client, repo ghrepo.Interface, number, comments int) (*Issue, error) { type response struct { Repository struct { Issue Issue @@ -329,7 +344,7 @@ func IssueByNumber(client *Client, repo ghrepo.Interface, number int) (*Issue, e } query := ` - query IssueByNumber($owner: String!, $repo: String!, $issue_number: Int!) { + query IssueByNumber($owner: String!, $repo: String!, $issue_number: Int!, $comments: Int!) { repository(owner: $owner, name: $repo) { hasIssuesEnabled issue(number: $issue_number) { @@ -341,7 +356,22 @@ func IssueByNumber(client *Client, repo ghrepo.Interface, number int) (*Issue, e author { login } - comments { + comments(last: $comments) { + nodes { + author { + login + } + authorAssociation + body + createdAt + includesCreatedEdit + reactionGroups { + content + users { + totalCount + } + } + } totalCount } number @@ -370,9 +400,15 @@ func IssueByNumber(client *Client, repo ghrepo.Interface, number int) (*Issue, e } totalCount } - milestone{ + milestone { title } + reactionGroups { + content + users { + totalCount + } + } } } }` @@ -381,6 +417,7 @@ func IssueByNumber(client *Client, repo ghrepo.Interface, number int) (*Issue, e "owner": repo.RepoOwner(), "repo": repo.RepoName(), "issue_number": number, + "comments": comments, } var resp response diff --git a/api/reaction_groups.go b/api/reaction_groups.go new file mode 100644 index 000000000..68c527079 --- /dev/null +++ b/api/reaction_groups.go @@ -0,0 +1,52 @@ +package api + +import ( + "fmt" + "strings" +) + +type ReactionGroups []ReactionGroup + +type ReactionGroup struct { + Content string + Users ReactionGroupUsers +} + +type ReactionGroupUsers struct { + TotalCount int +} + +func (rg ReactionGroup) String() string { + c := rg.Users.TotalCount + if c == 0 { + return "" + } + e := reactionEmoji[rg.Content] + if e == "" { + return "" + } + return fmt.Sprintf("%v %s", c, e) +} + +func (rgs ReactionGroups) String() string { + var rs []string + + for _, rg := range rgs { + if r := rg.String(); r != "" { + rs = append(rs, r) + } + } + + return strings.Join(rs, " • ") +} + +var reactionEmoji = map[string]string{ + "THUMBS_UP": "\U0001f44d", + "THUMBS_DOWN": "\U0001f44e", + "LAUGH": "\U0001f604", + "HOORAY": "\U0001f389", + "CONFUSED": "\U0001f615", + "HEART": "\u2764\ufe0f", + "ROCKET": "\U0001f680", + "EYES": "\U0001f440", +} diff --git a/api/reaction_groups_test.go b/api/reaction_groups_test.go new file mode 100644 index 000000000..f2fa7b1d4 --- /dev/null +++ b/api/reaction_groups_test.go @@ -0,0 +1,55 @@ +package api + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func Test_String(t *testing.T) { + tests := map[string]struct { + rgs ReactionGroups + output string + }{ + "empty reaction groups": { + rgs: []ReactionGroup{}, + output: `^$`, + }, + "non-empty reaction groups": { + rgs: []ReactionGroup{ + ReactionGroup{ + Content: "LAUGH", + Users: ReactionGroupUsers{TotalCount: 0}, + }, + ReactionGroup{ + Content: "HOORAY", + Users: ReactionGroupUsers{TotalCount: 1}, + }, + ReactionGroup{ + Content: "CONFUSED", + Users: ReactionGroupUsers{TotalCount: 0}, + }, + ReactionGroup{ + Content: "HEART", + Users: ReactionGroupUsers{TotalCount: 2}, + }, + }, + output: `^1 \x{1f389} • 2 \x{2764}\x{fe0f}$`, + }, + "reaction groups with unmapped emoji": { + rgs: []ReactionGroup{ + ReactionGroup{ + Content: "UNKNOWN", + Users: ReactionGroupUsers{TotalCount: 1}, + }, + }, + output: `^$`, + }, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + assert.Regexp(t, tt.output, tt.rgs.String()) + }) + } +} diff --git a/pkg/cmd/issue/shared/lookup.go b/pkg/cmd/issue/shared/lookup.go index 90a729599..09be5bab6 100644 --- a/pkg/cmd/issue/shared/lookup.go +++ b/pkg/cmd/issue/shared/lookup.go @@ -11,52 +11,55 @@ import ( "github.com/cli/cli/internal/ghrepo" ) -func IssueFromArg(apiClient *api.Client, baseRepoFn func() (ghrepo.Interface, error), arg string) (*api.Issue, ghrepo.Interface, error) { - issue, baseRepo, err := issueFromURL(apiClient, arg) - if err != nil { - return nil, nil, err - } - if issue != nil { - return issue, baseRepo, nil +func IssueWithCommentsFromArg(apiClient *api.Client, baseRepoFn func() (ghrepo.Interface, error), arg string, comments int) (*api.Issue, ghrepo.Interface, error) { + issueNumber, baseRepo := issueMetadataFromURL(arg) + + if baseRepo == nil { + var err error + baseRepo, err = baseRepoFn() + if err != nil { + return nil, nil, fmt.Errorf("could not determine base repo: %w", err) + } } - baseRepo, err = baseRepoFn() - if err != nil { - return nil, nil, fmt.Errorf("could not determine base repo: %w", err) + if issueNumber == 0 { + var err error + issueNumber, err = strconv.Atoi(strings.TrimPrefix(arg, "#")) + if err != nil { + return nil, nil, fmt.Errorf("invalid issue format: %q", arg) + } } - issueNumber, err := strconv.Atoi(strings.TrimPrefix(arg, "#")) - if err != nil { - return nil, nil, fmt.Errorf("invalid issue format: %q", arg) - } - - issue, err = issueFromNumber(apiClient, baseRepo, issueNumber) + issue, err := issueFromNumber(apiClient, baseRepo, issueNumber, comments) return issue, baseRepo, err } +func IssueFromArg(apiClient *api.Client, baseRepoFn func() (ghrepo.Interface, error), arg string) (*api.Issue, ghrepo.Interface, error) { + return IssueWithCommentsFromArg(apiClient, baseRepoFn, arg, 0) +} + var issueURLRE = regexp.MustCompile(`^/([^/]+)/([^/]+)/issues/(\d+)`) -func issueFromURL(apiClient *api.Client, s string) (*api.Issue, ghrepo.Interface, error) { +func issueMetadataFromURL(s string) (int, ghrepo.Interface) { u, err := url.Parse(s) if err != nil { - return nil, nil, nil + return 0, nil } if u.Scheme != "https" && u.Scheme != "http" { - return nil, nil, nil + return 0, nil } m := issueURLRE.FindStringSubmatch(u.Path) if m == nil { - return nil, nil, nil + return 0, nil } repo := ghrepo.NewWithHost(m[1], m[2], u.Hostname()) issueNumber, _ := strconv.Atoi(m[3]) - issue, err := issueFromNumber(apiClient, repo, issueNumber) - return issue, repo, err + return issueNumber, repo } -func issueFromNumber(apiClient *api.Client, repo ghrepo.Interface, issueNumber int) (*api.Issue, error) { - return api.IssueByNumber(apiClient, repo, issueNumber) +func issueFromNumber(apiClient *api.Client, repo ghrepo.Interface, issueNumber, comments int) (*api.Issue, error) { + return api.IssueByNumber(apiClient, repo, issueNumber, comments) } diff --git a/pkg/cmd/issue/view/fixtures/issueView_preview.json b/pkg/cmd/issue/view/fixtures/issueView_preview.json index e25090a61..65fc5ef51 100644 --- a/pkg/cmd/issue/view/fixtures/issueView_preview.json +++ b/pkg/cmd/issue/view/fixtures/issueView_preview.json @@ -7,21 +7,21 @@ "body": "**bold story**", "title": "ix of coins", "state": "OPEN", - "created_at": "2011-01-26T19:01:12Z", + "createdAt": "2011-01-26T19:01:12Z", "author": { "login": "marseilles" }, "assignees": { "nodes": [], - "totalcount": 0 + "totalCount": 0 }, "labels": { "nodes": [], - "totalcount": 0 + "totalCount": 0 }, "projectcards": { "nodes": [], - "totalcount": 0 + "totalCount": 0 }, "milestone": { "title": "" diff --git a/pkg/cmd/issue/view/fixtures/issueView_previewClosedState.json b/pkg/cmd/issue/view/fixtures/issueView_previewClosedState.json index 978927125..4665c47e1 100644 --- a/pkg/cmd/issue/view/fixtures/issueView_previewClosedState.json +++ b/pkg/cmd/issue/view/fixtures/issueView_previewClosedState.json @@ -7,7 +7,7 @@ "body": "**bold story**", "title": "ix of coins", "state": "CLOSED", - "created_at": "2011-01-26T19:01:12Z", + "createdAt": "2011-01-26T19:01:12Z", "author": { "login": "marseilles" }, diff --git a/pkg/cmd/issue/view/fixtures/issueView_previewFullComments.json b/pkg/cmd/issue/view/fixtures/issueView_previewFullComments.json new file mode 100644 index 000000000..2805c9694 --- /dev/null +++ b/pkg/cmd/issue/view/fixtures/issueView_previewFullComments.json @@ -0,0 +1,383 @@ +{ + "data": { + "repository": { + "hasIssuesEnabled": true, + "issue": { + "number": 123, + "body": "some body", + "title": "some title", + "state": "OPEN", + "createdAt": "2020-01-01T12:00:00Z", + "author": { + "login": "marseilles" + }, + "assignees": { + "nodes": [], + "totalCount": 0 + }, + "labels": { + "nodes": [], + "totalCount": 0 + }, + "projectcards": { + "nodes": [], + "totalCount": 0 + }, + "milestone": { + "title": "" + }, + "reactionGroups": [ + { + "content": "CONFUSED", + "users": { + "totalCount": 0 + } + }, + { + "content": "EYES", + "users": { + "totalCount": 0 + } + }, + { + "content": "HEART", + "users": { + "totalCount": 0 + } + }, + { + "content": "HOORAY", + "users": { + "totalCount": 0 + } + }, + { + "content": "LAUGH", + "users": { + "totalCount": 0 + } + }, + { + "content": "ROCKET", + "users": { + "totalCount": 0 + } + }, + { + "content": "THUMBS_DOWN", + "users": { + "totalCount": 0 + } + }, + { + "content": "THUMBS_UP", + "users": { + "totalCount": 0 + } + } + ], + "comments": { + "nodes": [ + { + "author": { + "login": "monalisa" + }, + "authorAssociation": "NONE", + "body": "Comment 1", + "createdAt": "2020-01-01T12:00:00Z", + "includesCreatedEdit": false, + "reactionGroups": [ + { + "content": "CONFUSED", + "users": { + "totalCount": 1 + } + }, + { + "content": "EYES", + "users": { + "totalCount": 2 + } + }, + { + "content": "HEART", + "users": { + "totalCount": 3 + } + }, + { + "content": "HOORAY", + "users": { + "totalCount": 4 + } + }, + { + "content": "LAUGH", + "users": { + "totalCount": 5 + } + }, + { + "content": "ROCKET", + "users": { + "totalCount": 6 + } + }, + { + "content": "THUMBS_DOWN", + "users": { + "totalCount": 7 + } + }, + { + "content": "THUMBS_UP", + "users": { + "totalCount": 8 + } + } + ] + }, + { + "author": { + "login": "johnnytest" + }, + "authorAssociation": "CONTRIBUTOR", + "body": "Comment 2", + "createdAt": "2020-01-01T12:00:00Z", + "includesCreatedEdit": false, + "reactionGroups": [ + { + "content": "CONFUSED", + "users": { + "totalCount": 0 + } + }, + { + "content": "EYES", + "users": { + "totalCount": 0 + } + }, + { + "content": "HEART", + "users": { + "totalCount": 0 + } + }, + { + "content": "HOORAY", + "users": { + "totalCount": 0 + } + }, + { + "content": "LAUGH", + "users": { + "totalCount": 0 + } + }, + { + "content": "ROCKET", + "users": { + "totalCount": 0 + } + }, + { + "content": "THUMBS_DOWN", + "users": { + "totalCount": 0 + } + }, + { + "content": "THUMBS_UP", + "users": { + "totalCount": 0 + } + } + ] + }, + { + "author": { + "login": "elvisp" + }, + "authorAssociation": "MEMBER", + "body": "Comment 3", + "createdAt": "2020-01-01T12:00:00Z", + "includesCreatedEdit": false, + "reactionGroups": [ + { + "content": "CONFUSED", + "users": { + "totalCount": 0 + } + }, + { + "content": "EYES", + "users": { + "totalCount": 0 + } + }, + { + "content": "HEART", + "users": { + "totalCount": 0 + } + }, + { + "content": "HOORAY", + "users": { + "totalCount": 0 + } + }, + { + "content": "LAUGH", + "users": { + "totalCount": 0 + } + }, + { + "content": "ROCKET", + "users": { + "totalCount": 0 + } + }, + { + "content": "THUMBS_DOWN", + "users": { + "totalCount": 0 + } + }, + { + "content": "THUMBS_UP", + "users": { + "totalCount": 0 + } + } + ] + }, + { + "author": { + "login": "loislane" + }, + "authorAssociation": "OWNER", + "body": "Comment 4", + "createdAt": "2020-01-01T12:00:00Z", + "includesCreatedEdit": false, + "reactionGroups": [ + { + "content": "CONFUSED", + "users": { + "totalCount": 0 + } + }, + { + "content": "EYES", + "users": { + "totalCount": 0 + } + }, + { + "content": "HEART", + "users": { + "totalCount": 0 + } + }, + { + "content": "HOORAY", + "users": { + "totalCount": 0 + } + }, + { + "content": "LAUGH", + "users": { + "totalCount": 0 + } + }, + { + "content": "ROCKET", + "users": { + "totalCount": 0 + } + }, + { + "content": "THUMBS_DOWN", + "users": { + "totalCount": 0 + } + }, + { + "content": "THUMBS_UP", + "users": { + "totalCount": 0 + } + } + ] + }, + { + "author": { + "login": "marseilles" + }, + "authorAssociation": "COLLABORATOR", + "body": "Comment 5", + "createdAt": "2020-01-01T12:00:00Z", + "includesCreatedEdit": false, + "reactionGroups": [ + { + "content": "CONFUSED", + "users": { + "totalCount": 0 + } + }, + { + "content": "EYES", + "users": { + "totalCount": 0 + } + }, + { + "content": "HEART", + "users": { + "totalCount": 0 + } + }, + { + "content": "HOORAY", + "users": { + "totalCount": 0 + } + }, + { + "content": "LAUGH", + "users": { + "totalCount": 0 + } + }, + { + "content": "ROCKET", + "users": { + "totalCount": 0 + } + }, + { + "content": "THUMBS_DOWN", + "users": { + "totalCount": 0 + } + }, + { + "content": "THUMBS_UP", + "users": { + "totalCount": 0 + } + } + ] + } + ], + "totalCount": 5 + }, + "url": "https://github.com/OWNER/REPO/issues/123" + } + } + } +} diff --git a/pkg/cmd/issue/view/fixtures/issueView_previewSingleComment.json b/pkg/cmd/issue/view/fixtures/issueView_previewSingleComment.json new file mode 100644 index 000000000..87fc7bffc --- /dev/null +++ b/pkg/cmd/issue/view/fixtures/issueView_previewSingleComment.json @@ -0,0 +1,147 @@ +{ + "data": { + "repository": { + "hasIssuesEnabled": true, + "issue": { + "number": 123, + "body": "some body", + "title": "some title", + "state": "OPEN", + "createdAt": "2020-01-01T12:00:00Z", + "author": { + "login": "marseilles" + }, + "assignees": { + "nodes": [], + "totalCount": 0 + }, + "labels": { + "nodes": [], + "totalCount": 0 + }, + "projectcards": { + "nodes": [], + "totalCount": 0 + }, + "milestone": { + "title": "" + }, + "reactionGroups": [ + { + "content": "CONFUSED", + "users": { + "totalCount": 0 + } + }, + { + "content": "EYES", + "users": { + "totalCount": 0 + } + }, + { + "content": "HEART", + "users": { + "totalCount": 0 + } + }, + { + "content": "HOORAY", + "users": { + "totalCount": 0 + } + }, + { + "content": "LAUGH", + "users": { + "totalCount": 0 + } + }, + { + "content": "ROCKET", + "users": { + "totalCount": 0 + } + }, + { + "content": "THUMBS_DOWN", + "users": { + "totalCount": 0 + } + }, + { + "content": "THUMBS_UP", + "users": { + "totalCount": 0 + } + } + ], + "comments": { + "nodes": [ + { + "author": { + "login": "marseilles" + }, + "authorAssociation": "COLLABORATOR", + "body": "Comment 5", + "createdAt": "2020-01-01T12:00:00Z", + "includesCreatedEdit": false, + "reactionGroups": [ + { + "content": "CONFUSED", + "users": { + "totalCount": 0 + } + }, + { + "content": "EYES", + "users": { + "totalCount": 0 + } + }, + { + "content": "HEART", + "users": { + "totalCount": 0 + } + }, + { + "content": "HOORAY", + "users": { + "totalCount": 0 + } + }, + { + "content": "LAUGH", + "users": { + "totalCount": 0 + } + }, + { + "content": "ROCKET", + "users": { + "totalCount": 0 + } + }, + { + "content": "THUMBS_DOWN", + "users": { + "totalCount": 0 + } + }, + { + "content": "THUMBS_UP", + "users": { + "totalCount": 0 + } + } + ] + } + ], + "totalCount": 5 + }, + "url": "https://github.com/OWNER/REPO/issues/123" + } + } + } +} diff --git a/pkg/cmd/issue/view/fixtures/issueView_previewThreeComments.json b/pkg/cmd/issue/view/fixtures/issueView_previewThreeComments.json new file mode 100644 index 000000000..74d5945e1 --- /dev/null +++ b/pkg/cmd/issue/view/fixtures/issueView_previewThreeComments.json @@ -0,0 +1,265 @@ +{ + "data": { + "repository": { + "hasIssuesEnabled": true, + "issue": { + "number": 123, + "body": "some body", + "title": "some title", + "state": "OPEN", + "createdAt": "2020-01-01T12:00:00Z", + "author": { + "login": "marseilles" + }, + "assignees": { + "nodes": [], + "totalCount": 0 + }, + "labels": { + "nodes": [], + "totalCount": 0 + }, + "projectcards": { + "nodes": [], + "totalCount": 0 + }, + "milestone": { + "title": "" + }, + "reactionGroups": [ + { + "content": "CONFUSED", + "users": { + "totalCount": 0 + } + }, + { + "content": "EYES", + "users": { + "totalCount": 0 + } + }, + { + "content": "HEART", + "users": { + "totalCount": 0 + } + }, + { + "content": "HOORAY", + "users": { + "totalCount": 0 + } + }, + { + "content": "LAUGH", + "users": { + "totalCount": 0 + } + }, + { + "content": "ROCKET", + "users": { + "totalCount": 0 + } + }, + { + "content": "THUMBS_DOWN", + "users": { + "totalCount": 0 + } + }, + { + "content": "THUMBS_UP", + "users": { + "totalCount": 0 + } + } + ], + "comments": { + "nodes": [ + { + "author": { + "login": "elvisp" + }, + "authorAssociation": "MEMBER", + "body": "Comment 3", + "createdAt": "2020-01-01T12:00:00Z", + "includesCreatedEdit": false, + "reactionGroups": [ + { + "content": "CONFUSED", + "users": { + "totalCount": 0 + } + }, + { + "content": "EYES", + "users": { + "totalCount": 0 + } + }, + { + "content": "HEART", + "users": { + "totalCount": 0 + } + }, + { + "content": "HOORAY", + "users": { + "totalCount": 0 + } + }, + { + "content": "LAUGH", + "users": { + "totalCount": 0 + } + }, + { + "content": "ROCKET", + "users": { + "totalCount": 0 + } + }, + { + "content": "THUMBS_DOWN", + "users": { + "totalCount": 0 + } + }, + { + "content": "THUMBS_UP", + "users": { + "totalCount": 0 + } + } + ] + }, + { + "author": { + "login": "loislane" + }, + "authorAssociation": "OWNER", + "body": "Comment 4", + "createdAt": "2020-01-01T12:00:00Z", + "includesCreatedEdit": false, + "reactionGroups": [ + { + "content": "CONFUSED", + "users": { + "totalCount": 0 + } + }, + { + "content": "EYES", + "users": { + "totalCount": 0 + } + }, + { + "content": "HEART", + "users": { + "totalCount": 0 + } + }, + { + "content": "HOORAY", + "users": { + "totalCount": 0 + } + }, + { + "content": "LAUGH", + "users": { + "totalCount": 0 + } + }, + { + "content": "ROCKET", + "users": { + "totalCount": 0 + } + }, + { + "content": "THUMBS_DOWN", + "users": { + "totalCount": 0 + } + }, + { + "content": "THUMBS_UP", + "users": { + "totalCount": 0 + } + } + ] + }, + { + "author": { + "login": "marseilles" + }, + "authorAssociation": "COLLABORATOR", + "body": "Comment 5", + "createdAt": "2020-01-01T12:00:00Z", + "includesCreatedEdit": false, + "reactionGroups": [ + { + "content": "CONFUSED", + "users": { + "totalCount": 0 + } + }, + { + "content": "EYES", + "users": { + "totalCount": 0 + } + }, + { + "content": "HEART", + "users": { + "totalCount": 0 + } + }, + { + "content": "HOORAY", + "users": { + "totalCount": 0 + } + }, + { + "content": "LAUGH", + "users": { + "totalCount": 0 + } + }, + { + "content": "ROCKET", + "users": { + "totalCount": 0 + } + }, + { + "content": "THUMBS_DOWN", + "users": { + "totalCount": 0 + } + }, + { + "content": "THUMBS_UP", + "users": { + "totalCount": 0 + } + } + ] + } + ], + "totalCount": 5 + }, + "url": "https://github.com/OWNER/REPO/issues/123" + } + } + } +} diff --git a/pkg/cmd/issue/view/fixtures/issueView_previewWithEmptyBody.json b/pkg/cmd/issue/view/fixtures/issueView_previewWithEmptyBody.json index 6e87a42b6..104f134be 100644 --- a/pkg/cmd/issue/view/fixtures/issueView_previewWithEmptyBody.json +++ b/pkg/cmd/issue/view/fixtures/issueView_previewWithEmptyBody.json @@ -7,7 +7,7 @@ "body": "", "title": "ix of coins", "state": "OPEN", - "created_at": "2011-01-26T19:01:12Z", + "createdAt": "2011-01-26T19:01:12Z", "author": { "login": "marseilles" }, diff --git a/pkg/cmd/issue/view/fixtures/issueView_previewWithMetadata.json b/pkg/cmd/issue/view/fixtures/issueView_previewWithMetadata.json index 246bbd77b..9bce5f745 100644 --- a/pkg/cmd/issue/view/fixtures/issueView_previewWithMetadata.json +++ b/pkg/cmd/issue/view/fixtures/issueView_previewWithMetadata.json @@ -7,7 +7,7 @@ "body": "**bold story**", "title": "ix of coins", "state": "OPEN", - "created_at": "2011-01-26T19:01:12Z", + "createdAt": "2011-01-26T19:01:12Z", "author": { "login": "marseilles" }, @@ -20,7 +20,7 @@ "login": "monaco" } ], - "totalcount": 2 + "totalCount": 2 }, "labels": { "nodes": [ @@ -40,7 +40,7 @@ "name": "five" } ], - "totalcount": 5 + "totalCount": 5 }, "projectcards": { "nodes": [ @@ -77,13 +77,63 @@ } } ], - "totalcount": 3 + "totalCount": 3 }, "milestone": { "title": "uluru" }, + "reactionGroups": [ + { + "content": "CONFUSED", + "users": { + "totalCount": 8 + } + }, + { + "content": "EYES", + "users": { + "totalCount": 7 + } + }, + { + "content": "HEART", + "users": { + "totalCount": 6 + } + }, + { + "content": "HOORAY", + "users": { + "totalCount": 5 + } + }, + { + "content": "LAUGH", + "users": { + "totalCount": 4 + } + }, + { + "content": "ROCKET", + "users": { + "totalCount": 3 + } + }, + { + "content": "THUMBS_DOWN", + "users": { + "totalCount": 2 + } + }, + { + "content": "THUMBS_UP", + "users": { + "totalCount": 1 + } + } + ], "comments": { - "totalcount": 9 + "totalCount": 9 }, "url": "https://github.com/OWNER/REPO/issues/123" } diff --git a/pkg/cmd/issue/view/view.go b/pkg/cmd/issue/view/view.go index 2bada530c..759789036 100644 --- a/pkg/cmd/issue/view/view.go +++ b/pkg/cmd/issue/view/view.go @@ -29,6 +29,7 @@ type ViewOptions struct { SelectorArg string WebMode bool + Comments int } func NewCmdView(f *cmdutil.Factory, runF func(*ViewOptions) error) *cobra.Command { @@ -65,6 +66,8 @@ func NewCmdView(f *cmdutil.Factory, runF func(*ViewOptions) error) *cobra.Comman } cmd.Flags().BoolVarP(&opts.WebMode, "web", "w", false, "Open an issue in the browser") + cmd.Flags().IntVarP(&opts.Comments, "comments", "c", 1, "View issue comments") + cmd.Flags().Lookup("comments").NoOptDefVal = "30" return cmd } @@ -76,7 +79,7 @@ func viewRun(opts *ViewOptions) error { } apiClient := api.NewClientFromHTTP(httpClient) - issue, _, err := issueShared.IssueFromArg(apiClient, opts.BaseRepo, opts.SelectorArg) + issue, _, err := issueShared.IssueWithCommentsFromArg(apiClient, opts.BaseRepo, opts.SelectorArg, opts.Comments) if err != nil { return err } @@ -122,9 +125,33 @@ func printRawIssuePreview(out io.Writer, issue *api.Issue) error { fmt.Fprintln(out, "--") fmt.Fprintln(out, issue.Body) + fmt.Fprintln(out, "--") + + if len(issue.Comments.Nodes) > 0 { + fmt.Fprintf(out, rawIssueComments(issue.Comments)) + } + return nil } +func rawIssueComments(comments api.IssueComments) string { + var b strings.Builder + for _, comment := range comments.Nodes { + fmt.Fprintf(&b, rawIssueComment(comment)) + } + return b.String() +} + +func rawIssueComment(comment api.IssueComment) string { + var b strings.Builder + fmt.Fprintf(&b, "author:\t%s\n", comment.Author.Login) + fmt.Fprintf(&b, "association:\t%s\n", strings.ToLower(comment.AuthorAssociation)) + fmt.Fprintln(&b, "--") + fmt.Fprintln(&b, comment.Body) + fmt.Fprintln(&b, "--") + return b.String() +} + func printHumanIssuePreview(io *iostreams.IOStreams, issue *api.Issue) error { out := io.Out now := time.Now() @@ -133,16 +160,21 @@ func printHumanIssuePreview(io *iostreams.IOStreams, issue *api.Issue) error { // Header (Title and State) fmt.Fprintln(out, cs.Bold(issue.Title)) - fmt.Fprint(out, issueStateTitleWithColor(cs, issue.State)) - fmt.Fprintln(out, cs.Gray(fmt.Sprintf( - " • %s opened %s • %s", + fmt.Fprintln(out, fmt.Sprintf( + "%s • %s opened %s • %s", + issueStateTitleWithColor(cs, issue.State), issue.Author.Login, utils.FuzzyAgo(ago), utils.Pluralize(issue.Comments.TotalCount, "comment"), - ))) + )) + + // Reactions + if reactions := issue.ReactionGroups.String(); reactions != "" { + fmt.Fprint(out, reactions) + fmt.Fprintln(out) + } // Metadata - fmt.Fprintln(out) if assignees := issueAssigneeList(*issue); assignees != "" { fmt.Fprint(out, cs.Bold("Assignees: ")) fmt.Fprintln(out, assignees) @@ -161,22 +193,102 @@ func printHumanIssuePreview(io *iostreams.IOStreams, issue *api.Issue) error { } // Body - if issue.Body != "" { - fmt.Fprintln(out) - style := markdown.GetStyle(io.TerminalTheme()) - md, err := markdown.Render(issue.Body, style, "") + fmt.Fprintln(out) + if issue.Body == "" { + issue.Body = "_No description provided_" + } + style := markdown.GetStyle(io.TerminalTheme()) + md, err := markdown.Render(issue.Body, style, "") + if err != nil { + return err + } + fmt.Fprint(out, md) + fmt.Fprintln(out) + + // Comments + if issue.Comments.TotalCount > 0 { + comments, err := issueComments(io, issue.Comments) if err != nil { return err } - fmt.Fprintln(out, md) + fmt.Fprintf(out, comments) } - fmt.Fprintln(out) // Footer - fmt.Fprintf(out, cs.Gray("View this issue on GitHub: %s\n"), issue.URL) + fmt.Fprintf(out, cs.Gray("View this issue on GitHub: %s"), issue.URL) + return nil } +func issueComments(io *iostreams.IOStreams, comments api.IssueComments) (string, error) { + var b strings.Builder + cs := io.ColorScheme() + retrievedCount := len(comments.Nodes) + hiddenCount := comments.TotalCount - retrievedCount + + if hiddenCount > 0 { + fmt.Fprintf(&b, cs.Gray(fmt.Sprintf("———————— Hiding %v comments ————————", hiddenCount))) + fmt.Fprintf(&b, "\n\n\n") + } + + for i, comment := range comments.Nodes { + last := i+1 == retrievedCount + cmt, err := issueComment(io, comment, last) + if err != nil { + return "", err + } + fmt.Fprintf(&b, cmt) + if last { + fmt.Fprintln(&b) + } + } + + if hiddenCount > 0 { + fmt.Fprintf(&b, cs.Gray("Use --comments to view the full conversation")) + fmt.Fprintln(&b) + } + + return b.String(), nil +} + +func issueComment(io *iostreams.IOStreams, comment api.IssueComment, newest bool) (string, error) { + var b strings.Builder + cs := io.ColorScheme() + + // Header + fmt.Fprintf(&b, cs.Bold(comment.Author.Login)) + if comment.AuthorAssociation != "NONE" { + fmt.Fprintf(&b, cs.Bold(fmt.Sprintf(" (%s)", strings.ToLower(comment.AuthorAssociation)))) + } + fmt.Fprintf(&b, cs.Bold(fmt.Sprintf(" • %s", utils.FuzzyAgoAbbr(time.Now(), comment.CreatedAt)))) + if comment.IncludesCreatedEdit { + fmt.Fprintf(&b, cs.Bold(" • edited")) + } + if newest { + fmt.Fprintf(&b, cs.Bold(" • ")) + fmt.Fprintf(&b, cs.CyanBold("Newest comment")) + } + fmt.Fprintln(&b) + + // Reactions + if reactions := comment.ReactionGroups.String(); reactions != "" { + fmt.Fprint(&b, reactions) + fmt.Fprintln(&b) + } + + // Body + if comment.Body != "" { + style := markdown.GetStyle(io.TerminalTheme()) + md, err := markdown.Render(comment.Body, style, "") + if err != nil { + return "", err + } + fmt.Fprint(&b, md) + } + + return b.String(), nil +} + func issueStateTitleWithColor(cs *iostreams.ColorScheme, state string) string { colorFunc := cs.ColorFromString(prShared.ColorForState(state)) return colorFunc(strings.Title(strings.ToLower(state))) diff --git a/pkg/cmd/issue/view/view_test.go b/pkg/cmd/issue/view/view_test.go index 0567c87ff..cb1b1604e 100644 --- a/pkg/cmd/issue/view/view_test.go +++ b/pkg/cmd/issue/view/view_test.go @@ -5,7 +5,6 @@ import ( "io/ioutil" "net/http" "os/exec" - "reflect" "testing" "github.com/cli/cli/internal/config" @@ -16,15 +15,9 @@ import ( "github.com/cli/cli/pkg/iostreams" "github.com/cli/cli/test" "github.com/google/shlex" + "github.com/stretchr/testify/assert" ) -func eq(t *testing.T, got interface{}, expected interface{}) { - t.Helper() - if !reflect.DeepEqual(got, expected) { - t.Errorf("expected: %v, got: %v", expected, got) - } -} - func runCommand(rt http.RoundTripper, isTTY bool, cli string) (*test.CmdOut, error) { io, _, stdout, stderr := iostreams.Test() io.SetStdoutTTY(isTTY) @@ -86,14 +79,14 @@ func TestIssueView_web(t *testing.T) { t.Errorf("error running command `issue view`: %v", err) } - eq(t, output.String(), "") - eq(t, output.Stderr(), "Opening github.com/OWNER/REPO/issues/123 in your browser.\n") + assert.Equal(t, "", output.String()) + assert.Equal(t, "Opening github.com/OWNER/REPO/issues/123 in your browser.\n", output.Stderr()) if seenCmd == nil { t.Fatal("expected a command to run") } url := seenCmd.Args[len(seenCmd.Args)-1] - eq(t, url, "https://github.com/OWNER/REPO/issues/123") + assert.Equal(t, "https://github.com/OWNER/REPO/issues/123", url) } func TestIssueView_web_numberArgWithHash(t *testing.T) { @@ -119,14 +112,14 @@ func TestIssueView_web_numberArgWithHash(t *testing.T) { t.Errorf("error running command `issue view`: %v", err) } - eq(t, output.String(), "") - eq(t, output.Stderr(), "Opening github.com/OWNER/REPO/issues/123 in your browser.\n") + assert.Equal(t, "", output.String()) + assert.Equal(t, "Opening github.com/OWNER/REPO/issues/123 in your browser.\n", output.Stderr()) if seenCmd == nil { t.Fatal("expected a command to run") } url := seenCmd.Args[len(seenCmd.Args)-1] - eq(t, url, "https://github.com/OWNER/REPO/issues/123") + assert.Equal(t, "https://github.com/OWNER/REPO/issues/123", url) } func TestIssueView_nontty_Preview(t *testing.T) { @@ -192,7 +185,7 @@ func TestIssueView_nontty_Preview(t *testing.T) { t.Errorf("error running `issue view`: %v", err) } - eq(t, output.Stderr(), "") + assert.Equal(t, "", output.Stderr()) test.ExpectLines(t, output.String(), tc.expectedOutputs...) }) @@ -208,7 +201,7 @@ func TestIssueView_tty_Preview(t *testing.T) { fixture: "./fixtures/issueView_preview.json", expectedOutputs: []string{ `ix of coins`, - `Open.*marseilles opened about 292 years ago.*9 comments`, + `Open.*marseilles opened about 9 years ago.*9 comments`, `bold story`, `View this issue on GitHub: https://github.com/OWNER/REPO/issues/123`, }, @@ -217,7 +210,8 @@ func TestIssueView_tty_Preview(t *testing.T) { fixture: "./fixtures/issueView_previewWithMetadata.json", expectedOutputs: []string{ `ix of coins`, - `Open.*marseilles opened about 292 years ago.*9 comments`, + `Open.*marseilles opened about 9 years ago.*9 comments`, + `8 \x{1f615} • 7 \x{1f440} • 6 \x{2764}\x{fe0f} • 5 \x{1f389} • 4 \x{1f604} • 3 \x{1f680} • 2 \x{1f44e} • 1 \x{1f44d}`, `Assignees:.*marseilles, monaco\n`, `Labels:.*one, two, three, four, five\n`, `Projects:.*Project 1 \(column A\), Project 2 \(column B\), Project 3 \(column C\), Project 4 \(Awaiting triage\)\n`, @@ -230,7 +224,8 @@ func TestIssueView_tty_Preview(t *testing.T) { fixture: "./fixtures/issueView_previewWithEmptyBody.json", expectedOutputs: []string{ `ix of coins`, - `Open.*marseilles opened about 292 years ago.*9 comments`, + `Open.*marseilles opened about 9 years ago.*9 comments`, + `No description provided`, `View this issue on GitHub: https://github.com/OWNER/REPO/issues/123`, }, }, @@ -238,7 +233,7 @@ func TestIssueView_tty_Preview(t *testing.T) { fixture: "./fixtures/issueView_previewClosedState.json", expectedOutputs: []string{ `ix of coins`, - `Closed.*marseilles opened about 292 years ago.*9 comments`, + `Closed.*marseilles opened about 9 years ago.*9 comments`, `bold story`, `View this issue on GitHub: https://github.com/OWNER/REPO/issues/123`, }, @@ -256,7 +251,7 @@ func TestIssueView_tty_Preview(t *testing.T) { t.Errorf("error running `issue view`: %v", err) } - eq(t, output.Stderr(), "") + assert.Equal(t, "", output.Stderr()) test.ExpectLines(t, output.String(), tc.expectedOutputs...) }) @@ -330,11 +325,182 @@ func TestIssueView_web_urlArg(t *testing.T) { t.Errorf("error running command `issue view`: %v", err) } - eq(t, output.String(), "") + assert.Equal(t, "", output.String()) if seenCmd == nil { t.Fatal("expected a command to run") } url := seenCmd.Args[len(seenCmd.Args)-1] - eq(t, url, "https://github.com/OWNER/REPO/issues/123") + assert.Equal(t, "https://github.com/OWNER/REPO/issues/123", url) +} + +func TestIssueView_tty_Comments(t *testing.T) { + tests := map[string]struct { + cli string + fixture string + expectedOutputs []string + wantsErr bool + }{ + "without comments flag": { + cli: "123", + fixture: "./fixtures/issueView_previewSingleComment.json", + expectedOutputs: []string{ + `some title`, + `some body`, + `———————— Hiding 4 comments ————————`, + `marseilles \(collaborator\) • Jan 1, 2020 • Newest comment`, + `Comment 5`, + `Use --comments to view the full conversation`, + `View this issue on GitHub: https://github.com/OWNER/REPO/issues/123`, + }, + }, + "with default comments flag": { + cli: "123 --comments", + fixture: "./fixtures/issueView_previewFullComments.json", + expectedOutputs: []string{ + `some title`, + `some body`, + `monalisa • Jan 1, 2020`, + `1 \x{1f615} • 2 \x{1f440} • 3 \x{2764}\x{fe0f} • 4 \x{1f389} • 5 \x{1f604} • 6 \x{1f680} • 7 \x{1f44e} • 8 \x{1f44d}`, + `Comment 1`, + `johnnytest \(contributor\) • Jan 1, 2020`, + `Comment 2`, + `elvisp \(member\) • Jan 1, 2020`, + `Comment 3`, + `loislane \(owner\) • Jan 1, 2020`, + `Comment 4`, + `marseilles \(collaborator\) • Jan 1, 2020 • Newest comment`, + `Comment 5`, + `View this issue on GitHub: https://github.com/OWNER/REPO/issues/123`, + }, + }, + "with specified comments flag": { + cli: "123 --comments=3", + fixture: "./fixtures/issueView_previewThreeComments.json", + expectedOutputs: []string{ + `some title`, + `some body`, + `———————— Hiding 2 comments ————————`, + `elvisp \(member\) • Jan 1, 2020`, + `Comment 3`, + `loislane \(owner\) • Jan 1, 2020`, + `Comment 4`, + `marseilles \(collaborator\) • Jan 1, 2020 • Newest comment`, + `Comment 5`, + `Use --comments to view the full conversation`, + `View this issue on GitHub: https://github.com/OWNER/REPO/issues/123`, + }, + }, + "with incorrect comments flag": { + cli: "123 --comments 3", + fixture: "", + wantsErr: true, + }, + } + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + http := &httpmock.Registry{} + defer http.Verify(t) + if tc.fixture != "" { + http.Register(httpmock.GraphQL(`query IssueByNumber\b`), httpmock.FileResponse(tc.fixture)) + } + output, err := runCommand(http, true, tc.cli) + if tc.wantsErr { + assert.Error(t, err) + return + } + assert.NoError(t, err) + assert.Equal(t, "", output.Stderr()) + test.ExpectLines(t, output.String(), tc.expectedOutputs...) + }) + } +} + +func TestIssueView_nontty_Comments(t *testing.T) { + tests := map[string]struct { + cli string + fixture string + expectedOutputs []string + wantsErr bool + }{ + "without comments flag": { + cli: "123", + fixture: "./fixtures/issueView_previewSingleComment.json", + expectedOutputs: []string{ + `title:\tsome title`, + `author:\tmarseilles`, + `comments:\t5`, + `some body`, + `author:\tmarseilles`, + `association:\tcollaborator`, + `Comment 5`, + }, + }, + "with default comments flag": { + cli: "123 --comments", + fixture: "./fixtures/issueView_previewFullComments.json", + expectedOutputs: []string{ + `title:\tsome title`, + `author:\tmarseilles`, + `comments:\t5`, + `some body`, + `author:\tmonalisa`, + `association:\t`, + `Comment 1`, + `author:\tjohnnytest`, + `association:\tcontributor`, + `Comment 2`, + `author:\telvisp`, + `association:\tmember`, + `Comment 3`, + `author:\tloislane`, + `association:\towner`, + `Comment 4`, + `author:\tmarseilles`, + `association:\tcollaborator`, + `Comment 5`, + }, + }, + "with specified comments flag": { + cli: "123 --comments=3", + fixture: "./fixtures/issueView_previewThreeComments.json", + expectedOutputs: []string{ + `title:\tsome title`, + `author:\tmarseilles`, + `comments:\t5`, + `some body`, + `author:\telvisp`, + `association:\tmember`, + `Comment 3`, + `author:\tloislane`, + `association:\towner`, + `Comment 4`, + `author:\tmarseilles`, + `association:\tcollaborator`, + `Comment 5`, + }, + }, + "with incorrect comments flag": { + cli: "123 --comments 3", + fixture: "", + wantsErr: true, + }, + } + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + http := &httpmock.Registry{} + defer http.Verify(t) + if tc.fixture != "" { + http.Register(httpmock.GraphQL(`query IssueByNumber\b`), httpmock.FileResponse(tc.fixture)) + } + output, err := runCommand(http, false, tc.cli) + if tc.wantsErr { + assert.Error(t, err) + return + } + assert.NoError(t, err) + assert.Equal(t, "", output.Stderr()) + test.ExpectLines(t, output.String(), tc.expectedOutputs...) + }) + } } diff --git a/pkg/iostreams/color.go b/pkg/iostreams/color.go index 6fc2bd023..972caab3d 100644 --- a/pkg/iostreams/color.go +++ b/pkg/iostreams/color.go @@ -9,14 +9,15 @@ import ( ) var ( - magenta = ansi.ColorFunc("magenta") - cyan = ansi.ColorFunc("cyan") - red = ansi.ColorFunc("red") - yellow = ansi.ColorFunc("yellow") - blue = ansi.ColorFunc("blue") - green = ansi.ColorFunc("green") - gray = ansi.ColorFunc("black+h") - bold = ansi.ColorFunc("default+b") + magenta = ansi.ColorFunc("magenta") + cyan = ansi.ColorFunc("cyan") + red = ansi.ColorFunc("red") + yellow = ansi.ColorFunc("yellow") + blue = ansi.ColorFunc("blue") + green = ansi.ColorFunc("green") + gray = ansi.ColorFunc("black+h") + bold = ansi.ColorFunc("default+b") + cyanBold = ansi.ColorFunc("cyan+b") gray256 = func(t string) string { return fmt.Sprintf("\x1b[%d;5;%dm%s\x1b[m", 38, 242, t) @@ -107,6 +108,13 @@ func (c *ColorScheme) Cyan(t string) string { return cyan(t) } +func (c *ColorScheme) CyanBold(t string) string { + if !c.enabled { + return t + } + return cyanBold(t) +} + func (c *ColorScheme) Blue(t string) string { if !c.enabled { return t diff --git a/utils/utils.go b/utils/utils.go index 602b8ab1e..4b58508ef 100644 --- a/utils/utils.go +++ b/utils/utils.go @@ -59,6 +59,22 @@ func FuzzyAgo(ago time.Duration) string { return fmtDuration(int(ago.Hours()/24/365), "year") } +func FuzzyAgoAbbr(now time.Time, createdAt time.Time) string { + ago := now.Sub(createdAt) + + if ago < time.Hour { + return fmt.Sprintf("%d%s", int(ago.Minutes()), "m") + } + if ago < 24*time.Hour { + return fmt.Sprintf("%d%s", int(ago.Hours()), "h") + } + if ago < 30*24*time.Hour { + return fmt.Sprintf("%d%s", int(ago.Hours())/24, "d") + } + + return createdAt.Format("Jan _2, 2006") +} + func Humanize(s string) string { // Replaces - and _ with spaces. replace := "_-" diff --git a/utils/utils_test.go b/utils/utils_test.go index 0891c2a39..5dc7b2478 100644 --- a/utils/utils_test.go +++ b/utils/utils_test.go @@ -6,7 +6,6 @@ import ( ) func TestFuzzyAgo(t *testing.T) { - cases := map[string]string{ "1s": "less than a minute ago", "30s": "less than a minute ago", @@ -36,3 +35,29 @@ func TestFuzzyAgo(t *testing.T) { } } } + +func TestFuzzyAgoAbbr(t *testing.T) { + const form = "2006-Jan-02 15:04:05" + now, _ := time.Parse(form, "2020-Nov-22 14:00:00") + + cases := map[string]string{ + "2020-Nov-22 14:00:00": "0m", + "2020-Nov-22 13:59:00": "1m", + "2020-Nov-22 13:30:00": "30m", + "2020-Nov-22 13:00:00": "1h", + "2020-Nov-22 02:00:00": "12h", + "2020-Nov-21 14:00:00": "1d", + "2020-Nov-07 14:00:00": "15d", + "2020-Oct-24 14:00:00": "29d", + "2020-Oct-23 14:00:00": "Oct 23, 2020", + "2019-Nov-22 14:00:00": "Nov 22, 2019", + } + + for createdAt, expected := range cases { + d, _ := time.Parse(form, createdAt) + fuzzy := FuzzyAgoAbbr(now, d) + if fuzzy != expected { + t.Errorf("unexpected fuzzy duration abbr value: %s for %s", fuzzy, createdAt) + } + } +} From 8c5e5a382094551df9ea768e0fc8b7bfb0a52472 Mon Sep 17 00:00:00 2001 From: Sam Coe Date: Mon, 23 Nov 2020 18:37:27 +0300 Subject: [PATCH 2/6] Appease the linter --- api/reaction_groups_test.go | 10 +++++----- pkg/cmd/issue/view/view.go | 30 +++++++++++++++--------------- 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/api/reaction_groups_test.go b/api/reaction_groups_test.go index f2fa7b1d4..4e501c338 100644 --- a/api/reaction_groups_test.go +++ b/api/reaction_groups_test.go @@ -17,19 +17,19 @@ func Test_String(t *testing.T) { }, "non-empty reaction groups": { rgs: []ReactionGroup{ - ReactionGroup{ + { Content: "LAUGH", Users: ReactionGroupUsers{TotalCount: 0}, }, - ReactionGroup{ + { Content: "HOORAY", Users: ReactionGroupUsers{TotalCount: 1}, }, - ReactionGroup{ + { Content: "CONFUSED", Users: ReactionGroupUsers{TotalCount: 0}, }, - ReactionGroup{ + { Content: "HEART", Users: ReactionGroupUsers{TotalCount: 2}, }, @@ -38,7 +38,7 @@ func Test_String(t *testing.T) { }, "reaction groups with unmapped emoji": { rgs: []ReactionGroup{ - ReactionGroup{ + { Content: "UNKNOWN", Users: ReactionGroupUsers{TotalCount: 1}, }, diff --git a/pkg/cmd/issue/view/view.go b/pkg/cmd/issue/view/view.go index 759789036..9e81115f8 100644 --- a/pkg/cmd/issue/view/view.go +++ b/pkg/cmd/issue/view/view.go @@ -128,7 +128,7 @@ func printRawIssuePreview(out io.Writer, issue *api.Issue) error { fmt.Fprintln(out, "--") if len(issue.Comments.Nodes) > 0 { - fmt.Fprintf(out, rawIssueComments(issue.Comments)) + fmt.Fprint(out, rawIssueComments(issue.Comments)) } return nil @@ -137,7 +137,7 @@ func printRawIssuePreview(out io.Writer, issue *api.Issue) error { func rawIssueComments(comments api.IssueComments) string { var b strings.Builder for _, comment := range comments.Nodes { - fmt.Fprintf(&b, rawIssueComment(comment)) + fmt.Fprint(&b, rawIssueComment(comment)) } return b.String() } @@ -160,13 +160,13 @@ func printHumanIssuePreview(io *iostreams.IOStreams, issue *api.Issue) error { // Header (Title and State) fmt.Fprintln(out, cs.Bold(issue.Title)) - fmt.Fprintln(out, fmt.Sprintf( - "%s • %s opened %s • %s", + fmt.Fprintf(out, + "%s • %s opened %s • %s\n", issueStateTitleWithColor(cs, issue.State), issue.Author.Login, utils.FuzzyAgo(ago), utils.Pluralize(issue.Comments.TotalCount, "comment"), - )) + ) // Reactions if reactions := issue.ReactionGroups.String(); reactions != "" { @@ -211,7 +211,7 @@ func printHumanIssuePreview(io *iostreams.IOStreams, issue *api.Issue) error { if err != nil { return err } - fmt.Fprintf(out, comments) + fmt.Fprint(out, comments) } // Footer @@ -227,7 +227,7 @@ func issueComments(io *iostreams.IOStreams, comments api.IssueComments) (string, hiddenCount := comments.TotalCount - retrievedCount if hiddenCount > 0 { - fmt.Fprintf(&b, cs.Gray(fmt.Sprintf("———————— Hiding %v comments ————————", hiddenCount))) + fmt.Fprint(&b, cs.Gray(fmt.Sprintf("———————— Hiding %v comments ————————", hiddenCount))) fmt.Fprintf(&b, "\n\n\n") } @@ -237,14 +237,14 @@ func issueComments(io *iostreams.IOStreams, comments api.IssueComments) (string, if err != nil { return "", err } - fmt.Fprintf(&b, cmt) + fmt.Fprint(&b, cmt) if last { fmt.Fprintln(&b) } } if hiddenCount > 0 { - fmt.Fprintf(&b, cs.Gray("Use --comments to view the full conversation")) + fmt.Fprint(&b, cs.Gray("Use --comments to view the full conversation")) fmt.Fprintln(&b) } @@ -256,17 +256,17 @@ func issueComment(io *iostreams.IOStreams, comment api.IssueComment, newest bool cs := io.ColorScheme() // Header - fmt.Fprintf(&b, cs.Bold(comment.Author.Login)) + fmt.Fprint(&b, cs.Bold(comment.Author.Login)) if comment.AuthorAssociation != "NONE" { - fmt.Fprintf(&b, cs.Bold(fmt.Sprintf(" (%s)", strings.ToLower(comment.AuthorAssociation)))) + fmt.Fprint(&b, cs.Bold(fmt.Sprintf(" (%s)", strings.ToLower(comment.AuthorAssociation)))) } - fmt.Fprintf(&b, cs.Bold(fmt.Sprintf(" • %s", utils.FuzzyAgoAbbr(time.Now(), comment.CreatedAt)))) + fmt.Fprint(&b, cs.Bold(fmt.Sprintf(" • %s", utils.FuzzyAgoAbbr(time.Now(), comment.CreatedAt)))) if comment.IncludesCreatedEdit { - fmt.Fprintf(&b, cs.Bold(" • edited")) + fmt.Fprint(&b, cs.Bold(" • edited")) } if newest { - fmt.Fprintf(&b, cs.Bold(" • ")) - fmt.Fprintf(&b, cs.CyanBold("Newest comment")) + fmt.Fprint(&b, cs.Bold(" • ")) + fmt.Fprint(&b, cs.CyanBold("Newest comment")) } fmt.Fprintln(&b) From bad5a5942761eaeebb51356079e99c9c4d757ace Mon Sep 17 00:00:00 2001 From: Sam Coe Date: Tue, 24 Nov 2020 11:18:20 +0300 Subject: [PATCH 3/6] Update non-tty comment output --- .../issueView_previewFullComments.json | 2 +- pkg/cmd/issue/view/view.go | 31 ++++++++++--------- pkg/cmd/issue/view/view_test.go | 22 ++++++------- 3 files changed, 28 insertions(+), 27 deletions(-) diff --git a/pkg/cmd/issue/view/fixtures/issueView_previewFullComments.json b/pkg/cmd/issue/view/fixtures/issueView_previewFullComments.json index 2805c9694..78fe07597 100644 --- a/pkg/cmd/issue/view/fixtures/issueView_previewFullComments.json +++ b/pkg/cmd/issue/view/fixtures/issueView_previewFullComments.json @@ -85,7 +85,7 @@ "authorAssociation": "NONE", "body": "Comment 1", "createdAt": "2020-01-01T12:00:00Z", - "includesCreatedEdit": false, + "includesCreatedEdit": true, "reactionGroups": [ { "content": "CONFUSED", diff --git a/pkg/cmd/issue/view/view.go b/pkg/cmd/issue/view/view.go index 9e81115f8..b1f4cba82 100644 --- a/pkg/cmd/issue/view/view.go +++ b/pkg/cmd/issue/view/view.go @@ -27,9 +27,10 @@ type ViewOptions struct { IO *iostreams.IOStreams BaseRepo func() (ghrepo.Interface, error) - SelectorArg string - WebMode bool - Comments int + SelectorArg string + WebMode bool + Comments int + CommentsProvided bool } func NewCmdView(f *cmdutil.Factory, runF func(*ViewOptions) error) *cobra.Command { @@ -54,6 +55,8 @@ func NewCmdView(f *cmdutil.Factory, runF func(*ViewOptions) error) *cobra.Comman // support `-R, --repo` override opts.BaseRepo = f.BaseRepo + opts.CommentsProvided = cmd.Flags().Changed("comments") + if len(args) > 0 { opts.SelectorArg = args[0] } @@ -66,7 +69,7 @@ func NewCmdView(f *cmdutil.Factory, runF func(*ViewOptions) error) *cobra.Comman } cmd.Flags().BoolVarP(&opts.WebMode, "web", "w", false, "Open an issue in the browser") - cmd.Flags().IntVarP(&opts.Comments, "comments", "c", 1, "View issue comments") + cmd.Flags().IntVarP(&opts.Comments, "comments", "c", 1, "View issue comments sorted by most recent") cmd.Flags().Lookup("comments").NoOptDefVal = "30" return cmd @@ -104,6 +107,11 @@ func viewRun(opts *ViewOptions) error { if opts.IO.IsStdoutTTY() { return printHumanIssuePreview(opts.IO, issue) } + + if opts.CommentsProvided { + return printRawIssueComments(opts.IO.Out, issue) + } + return printRawIssuePreview(opts.IO.Out, issue) } @@ -122,30 +130,25 @@ func printRawIssuePreview(out io.Writer, issue *api.Issue) error { fmt.Fprintf(out, "assignees:\t%s\n", assignees) fmt.Fprintf(out, "projects:\t%s\n", projects) fmt.Fprintf(out, "milestone:\t%s\n", issue.Milestone.Title) - fmt.Fprintln(out, "--") fmt.Fprintln(out, issue.Body) - fmt.Fprintln(out, "--") - - if len(issue.Comments.Nodes) > 0 { - fmt.Fprint(out, rawIssueComments(issue.Comments)) - } - return nil } -func rawIssueComments(comments api.IssueComments) string { +func printRawIssueComments(out io.Writer, issue *api.Issue) error { var b strings.Builder - for _, comment := range comments.Nodes { + for _, comment := range issue.Comments.Nodes { fmt.Fprint(&b, rawIssueComment(comment)) } - return b.String() + fmt.Fprint(out, b.String()) + return nil } func rawIssueComment(comment api.IssueComment) string { var b strings.Builder fmt.Fprintf(&b, "author:\t%s\n", comment.Author.Login) fmt.Fprintf(&b, "association:\t%s\n", strings.ToLower(comment.AuthorAssociation)) + fmt.Fprintf(&b, "edited:\t%t\n", comment.IncludesCreatedEdit) fmt.Fprintln(&b, "--") fmt.Fprintln(&b, comment.Body) fmt.Fprintln(&b, "--") diff --git a/pkg/cmd/issue/view/view_test.go b/pkg/cmd/issue/view/view_test.go index cb1b1604e..977e73dad 100644 --- a/pkg/cmd/issue/view/view_test.go +++ b/pkg/cmd/issue/view/view_test.go @@ -360,7 +360,7 @@ func TestIssueView_tty_Comments(t *testing.T) { expectedOutputs: []string{ `some title`, `some body`, - `monalisa • Jan 1, 2020`, + `monalisa • Jan 1, 2020 • edited`, `1 \x{1f615} • 2 \x{1f440} • 3 \x{2764}\x{fe0f} • 4 \x{1f389} • 5 \x{1f604} • 6 \x{1f680} • 7 \x{1f44e} • 8 \x{1f44d}`, `Comment 1`, `johnnytest \(contributor\) • Jan 1, 2020`, @@ -428,36 +428,35 @@ func TestIssueView_nontty_Comments(t *testing.T) { fixture: "./fixtures/issueView_previewSingleComment.json", expectedOutputs: []string{ `title:\tsome title`, + `state:\tOPEN`, `author:\tmarseilles`, `comments:\t5`, `some body`, - `author:\tmarseilles`, - `association:\tcollaborator`, - `Comment 5`, }, }, "with default comments flag": { cli: "123 --comments", fixture: "./fixtures/issueView_previewFullComments.json", expectedOutputs: []string{ - `title:\tsome title`, - `author:\tmarseilles`, - `comments:\t5`, - `some body`, `author:\tmonalisa`, `association:\t`, + `edited:\ttrue`, `Comment 1`, `author:\tjohnnytest`, `association:\tcontributor`, + `edited:\tfalse`, `Comment 2`, `author:\telvisp`, `association:\tmember`, + `edited:\tfalse`, `Comment 3`, `author:\tloislane`, `association:\towner`, + `edited:\tfalse`, `Comment 4`, `author:\tmarseilles`, `association:\tcollaborator`, + `edited:\tfalse`, `Comment 5`, }, }, @@ -465,18 +464,17 @@ func TestIssueView_nontty_Comments(t *testing.T) { cli: "123 --comments=3", fixture: "./fixtures/issueView_previewThreeComments.json", expectedOutputs: []string{ - `title:\tsome title`, - `author:\tmarseilles`, - `comments:\t5`, - `some body`, `author:\telvisp`, `association:\tmember`, + `edited:\tfalse`, `Comment 3`, `author:\tloislane`, `association:\towner`, + `edited:\tfalse`, `Comment 4`, `author:\tmarseilles`, `association:\tcollaborator`, + `edited:\tfalse`, `Comment 5`, }, }, From bec5e0cd77782587bc9ac4ab653107a939972b9c Mon Sep 17 00:00:00 2001 From: Sam Coe Date: Mon, 7 Dec 2020 15:02:43 -0500 Subject: [PATCH 4/6] Address PR comments --- api/queries_comments.go | 65 +++++ api/queries_issue.go | 23 +- api/reaction_groups.go | 29 +- api/reaction_groups_test.go | 109 ++++--- pkg/cmd/issue/shared/lookup.go | 12 +- .../issueView_previewFullComments.json | 77 +---- .../issueView_previewThreeComments.json | 265 ------------------ pkg/cmd/issue/view/view.go | 78 ++++-- pkg/cmd/issue/view/view_test.go | 96 +++---- 9 files changed, 253 insertions(+), 501 deletions(-) create mode 100644 api/queries_comments.go delete mode 100644 pkg/cmd/issue/view/fixtures/issueView_previewThreeComments.json diff --git a/api/queries_comments.go b/api/queries_comments.go new file mode 100644 index 000000000..70ea382a1 --- /dev/null +++ b/api/queries_comments.go @@ -0,0 +1,65 @@ +package api + +import ( + "context" + "time" + + "github.com/cli/cli/internal/ghrepo" + "github.com/shurcooL/githubv4" +) + +type Comments struct { + Nodes []Comment + TotalCount int + PageInfo PageInfo +} + +type Comment struct { + Author Author + AuthorAssociation string + Body string + CreatedAt time.Time + IncludesCreatedEdit bool + ReactionGroups ReactionGroups +} + +type PageInfo struct { + HasNextPage bool + EndCursor string +} + +func CommentsForIssue(client *Client, repo ghrepo.Interface, issue *Issue) (*Comments, error) { + type response struct { + Repository struct { + Issue struct { + Comments Comments `graphql:"comments(first: 100, after: $endCursor)"` + } `graphql:"issue(number: $number)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + } + + variables := map[string]interface{}{ + "owner": githubv4.String(repo.RepoOwner()), + "repo": githubv4.String(repo.RepoName()), + "number": githubv4.Int(issue.Number), + "endCursor": (*githubv4.String)(nil), + } + + gql := graphQLClient(client.http, repo.RepoHost()) + + var comments []Comment + for { + var query response + err := gql.QueryNamed(context.Background(), "CommentsForIssue", &query, variables) + if err != nil { + return nil, err + } + + comments = append(comments, query.Repository.Issue.Comments.Nodes...) + if !query.Repository.Issue.Comments.PageInfo.HasNextPage { + break + } + variables["endCursor"] = githubv4.String(query.Repository.Issue.Comments.PageInfo.EndCursor) + } + + return &Comments{Nodes: comments, TotalCount: len(comments)}, nil +} diff --git a/api/queries_issue.go b/api/queries_issue.go index 1aa83ad89..17f32eabd 100644 --- a/api/queries_issue.go +++ b/api/queries_issue.go @@ -33,7 +33,7 @@ type Issue struct { Body string CreatedAt time.Time UpdatedAt time.Time - Comments IssueComments + Comments Comments Author Author Assignees struct { Nodes []struct { @@ -68,20 +68,6 @@ type IssuesDisabledError struct { error } -type IssueComments struct { - Nodes []IssueComment - TotalCount int -} - -type IssueComment struct { - Author Author - AuthorAssociation string - Body string - CreatedAt time.Time - IncludesCreatedEdit bool - ReactionGroups ReactionGroups -} - type Author struct { Login string } @@ -335,7 +321,7 @@ loop: return &res, nil } -func IssueByNumber(client *Client, repo ghrepo.Interface, number, comments int) (*Issue, error) { +func IssueByNumber(client *Client, repo ghrepo.Interface, number int) (*Issue, error) { type response struct { Repository struct { Issue Issue @@ -344,7 +330,7 @@ func IssueByNumber(client *Client, repo ghrepo.Interface, number, comments int) } query := ` - query IssueByNumber($owner: String!, $repo: String!, $issue_number: Int!, $comments: Int!) { + query IssueByNumber($owner: String!, $repo: String!, $issue_number: Int!) { repository(owner: $owner, name: $repo) { hasIssuesEnabled issue(number: $issue_number) { @@ -356,7 +342,7 @@ func IssueByNumber(client *Client, repo ghrepo.Interface, number, comments int) author { login } - comments(last: $comments) { + comments(last: 1) { nodes { author { login @@ -417,7 +403,6 @@ func IssueByNumber(client *Client, repo ghrepo.Interface, number, comments int) "owner": repo.RepoOwner(), "repo": repo.RepoName(), "issue_number": number, - "comments": comments, } var resp response diff --git a/api/reaction_groups.go b/api/reaction_groups.go index 68c527079..381504dd7 100644 --- a/api/reaction_groups.go +++ b/api/reaction_groups.go @@ -1,10 +1,5 @@ package api -import ( - "fmt" - "strings" -) - type ReactionGroups []ReactionGroup type ReactionGroup struct { @@ -16,28 +11,12 @@ type ReactionGroupUsers struct { TotalCount int } -func (rg ReactionGroup) String() string { - c := rg.Users.TotalCount - if c == 0 { - return "" - } - e := reactionEmoji[rg.Content] - if e == "" { - return "" - } - return fmt.Sprintf("%v %s", c, e) +func (rg ReactionGroup) Count() int { + return rg.Users.TotalCount } -func (rgs ReactionGroups) String() string { - var rs []string - - for _, rg := range rgs { - if r := rg.String(); r != "" { - rs = append(rs, r) - } - } - - return strings.Join(rs, " • ") +func (rg ReactionGroup) Emoji() string { + return reactionEmoji[rg.Content] } var reactionEmoji = map[string]string{ diff --git a/api/reaction_groups_test.go b/api/reaction_groups_test.go index 4e501c338..e30a9e1f8 100644 --- a/api/reaction_groups_test.go +++ b/api/reaction_groups_test.go @@ -8,48 +8,93 @@ import ( func Test_String(t *testing.T) { tests := map[string]struct { - rgs ReactionGroups - output string + rg ReactionGroup + emoji string + count int }{ - "empty reaction groups": { - rgs: []ReactionGroup{}, - output: `^$`, + "empty reaction group": { + rg: ReactionGroup{}, + emoji: "", + count: 0, }, - "non-empty reaction groups": { - rgs: []ReactionGroup{ - { - Content: "LAUGH", - Users: ReactionGroupUsers{TotalCount: 0}, - }, - { - Content: "HOORAY", - Users: ReactionGroupUsers{TotalCount: 1}, - }, - { - Content: "CONFUSED", - Users: ReactionGroupUsers{TotalCount: 0}, - }, - { - Content: "HEART", - Users: ReactionGroupUsers{TotalCount: 2}, - }, + "unknown reaction group": { + rg: ReactionGroup{ + Content: "UNKNOWN", + Users: ReactionGroupUsers{TotalCount: 1}, }, - output: `^1 \x{1f389} • 2 \x{2764}\x{fe0f}$`, + emoji: "", + count: 1, }, - "reaction groups with unmapped emoji": { - rgs: []ReactionGroup{ - { - Content: "UNKNOWN", - Users: ReactionGroupUsers{TotalCount: 1}, - }, + "thumbs up reaction group": { + rg: ReactionGroup{ + Content: "THUMBS_UP", + Users: ReactionGroupUsers{TotalCount: 2}, }, - output: `^$`, + emoji: "\U0001f44d", + count: 2, + }, + "thumbs down reaction group": { + rg: ReactionGroup{ + Content: "THUMBS_DOWN", + Users: ReactionGroupUsers{TotalCount: 3}, + }, + emoji: "\U0001f44e", + count: 3, + }, + "laugh reaction group": { + rg: ReactionGroup{ + Content: "LAUGH", + Users: ReactionGroupUsers{TotalCount: 4}, + }, + emoji: "\U0001f604", + count: 4, + }, + "hooray reaction group": { + rg: ReactionGroup{ + Content: "HOORAY", + Users: ReactionGroupUsers{TotalCount: 5}, + }, + emoji: "\U0001f389", + count: 5, + }, + "confused reaction group": { + rg: ReactionGroup{ + Content: "CONFUSED", + Users: ReactionGroupUsers{TotalCount: 6}, + }, + emoji: "\U0001f615", + count: 6, + }, + "heart reaction group": { + rg: ReactionGroup{ + Content: "HEART", + Users: ReactionGroupUsers{TotalCount: 7}, + }, + emoji: "\u2764\ufe0f", + count: 7, + }, + "rocket reaction group": { + rg: ReactionGroup{ + Content: "ROCKET", + Users: ReactionGroupUsers{TotalCount: 8}, + }, + emoji: "\U0001f680", + count: 8, + }, + "eyes reaction group": { + rg: ReactionGroup{ + Content: "EYES", + Users: ReactionGroupUsers{TotalCount: 9}, + }, + emoji: "\U0001f440", + count: 9, }, } for name, tt := range tests { t.Run(name, func(t *testing.T) { - assert.Regexp(t, tt.output, tt.rgs.String()) + assert.Equal(t, tt.emoji, tt.rg.Emoji()) + assert.Equal(t, tt.count, tt.rg.Count()) }) } } diff --git a/pkg/cmd/issue/shared/lookup.go b/pkg/cmd/issue/shared/lookup.go index 09be5bab6..675574f7c 100644 --- a/pkg/cmd/issue/shared/lookup.go +++ b/pkg/cmd/issue/shared/lookup.go @@ -11,7 +11,7 @@ import ( "github.com/cli/cli/internal/ghrepo" ) -func IssueWithCommentsFromArg(apiClient *api.Client, baseRepoFn func() (ghrepo.Interface, error), arg string, comments int) (*api.Issue, ghrepo.Interface, error) { +func IssueFromArg(apiClient *api.Client, baseRepoFn func() (ghrepo.Interface, error), arg string) (*api.Issue, ghrepo.Interface, error) { issueNumber, baseRepo := issueMetadataFromURL(arg) if baseRepo == nil { @@ -30,14 +30,10 @@ func IssueWithCommentsFromArg(apiClient *api.Client, baseRepoFn func() (ghrepo.I } } - issue, err := issueFromNumber(apiClient, baseRepo, issueNumber, comments) + issue, err := issueFromNumber(apiClient, baseRepo, issueNumber) return issue, baseRepo, err } -func IssueFromArg(apiClient *api.Client, baseRepoFn func() (ghrepo.Interface, error), arg string) (*api.Issue, ghrepo.Interface, error) { - return IssueWithCommentsFromArg(apiClient, baseRepoFn, arg, 0) -} - var issueURLRE = regexp.MustCompile(`^/([^/]+)/([^/]+)/issues/(\d+)`) func issueMetadataFromURL(s string) (int, ghrepo.Interface) { @@ -60,6 +56,6 @@ func issueMetadataFromURL(s string) (int, ghrepo.Interface) { return issueNumber, repo } -func issueFromNumber(apiClient *api.Client, repo ghrepo.Interface, issueNumber, comments int) (*api.Issue, error) { - return api.IssueByNumber(apiClient, repo, issueNumber, comments) +func issueFromNumber(apiClient *api.Client, repo ghrepo.Interface, issueNumber int) (*api.Issue, error) { + return api.IssueByNumber(apiClient, repo, issueNumber) } diff --git a/pkg/cmd/issue/view/fixtures/issueView_previewFullComments.json b/pkg/cmd/issue/view/fixtures/issueView_previewFullComments.json index 78fe07597..d2cd27f30 100644 --- a/pkg/cmd/issue/view/fixtures/issueView_previewFullComments.json +++ b/pkg/cmd/issue/view/fixtures/issueView_previewFullComments.json @@ -1,81 +1,7 @@ { "data": { "repository": { - "hasIssuesEnabled": true, "issue": { - "number": 123, - "body": "some body", - "title": "some title", - "state": "OPEN", - "createdAt": "2020-01-01T12:00:00Z", - "author": { - "login": "marseilles" - }, - "assignees": { - "nodes": [], - "totalCount": 0 - }, - "labels": { - "nodes": [], - "totalCount": 0 - }, - "projectcards": { - "nodes": [], - "totalCount": 0 - }, - "milestone": { - "title": "" - }, - "reactionGroups": [ - { - "content": "CONFUSED", - "users": { - "totalCount": 0 - } - }, - { - "content": "EYES", - "users": { - "totalCount": 0 - } - }, - { - "content": "HEART", - "users": { - "totalCount": 0 - } - }, - { - "content": "HOORAY", - "users": { - "totalCount": 0 - } - }, - { - "content": "LAUGH", - "users": { - "totalCount": 0 - } - }, - { - "content": "ROCKET", - "users": { - "totalCount": 0 - } - }, - { - "content": "THUMBS_DOWN", - "users": { - "totalCount": 0 - } - }, - { - "content": "THUMBS_UP", - "users": { - "totalCount": 0 - } - } - ], "comments": { "nodes": [ { @@ -375,8 +301,7 @@ } ], "totalCount": 5 - }, - "url": "https://github.com/OWNER/REPO/issues/123" + } } } } diff --git a/pkg/cmd/issue/view/fixtures/issueView_previewThreeComments.json b/pkg/cmd/issue/view/fixtures/issueView_previewThreeComments.json deleted file mode 100644 index 74d5945e1..000000000 --- a/pkg/cmd/issue/view/fixtures/issueView_previewThreeComments.json +++ /dev/null @@ -1,265 +0,0 @@ -{ - "data": { - "repository": { - "hasIssuesEnabled": true, - "issue": { - "number": 123, - "body": "some body", - "title": "some title", - "state": "OPEN", - "createdAt": "2020-01-01T12:00:00Z", - "author": { - "login": "marseilles" - }, - "assignees": { - "nodes": [], - "totalCount": 0 - }, - "labels": { - "nodes": [], - "totalCount": 0 - }, - "projectcards": { - "nodes": [], - "totalCount": 0 - }, - "milestone": { - "title": "" - }, - "reactionGroups": [ - { - "content": "CONFUSED", - "users": { - "totalCount": 0 - } - }, - { - "content": "EYES", - "users": { - "totalCount": 0 - } - }, - { - "content": "HEART", - "users": { - "totalCount": 0 - } - }, - { - "content": "HOORAY", - "users": { - "totalCount": 0 - } - }, - { - "content": "LAUGH", - "users": { - "totalCount": 0 - } - }, - { - "content": "ROCKET", - "users": { - "totalCount": 0 - } - }, - { - "content": "THUMBS_DOWN", - "users": { - "totalCount": 0 - } - }, - { - "content": "THUMBS_UP", - "users": { - "totalCount": 0 - } - } - ], - "comments": { - "nodes": [ - { - "author": { - "login": "elvisp" - }, - "authorAssociation": "MEMBER", - "body": "Comment 3", - "createdAt": "2020-01-01T12:00:00Z", - "includesCreatedEdit": false, - "reactionGroups": [ - { - "content": "CONFUSED", - "users": { - "totalCount": 0 - } - }, - { - "content": "EYES", - "users": { - "totalCount": 0 - } - }, - { - "content": "HEART", - "users": { - "totalCount": 0 - } - }, - { - "content": "HOORAY", - "users": { - "totalCount": 0 - } - }, - { - "content": "LAUGH", - "users": { - "totalCount": 0 - } - }, - { - "content": "ROCKET", - "users": { - "totalCount": 0 - } - }, - { - "content": "THUMBS_DOWN", - "users": { - "totalCount": 0 - } - }, - { - "content": "THUMBS_UP", - "users": { - "totalCount": 0 - } - } - ] - }, - { - "author": { - "login": "loislane" - }, - "authorAssociation": "OWNER", - "body": "Comment 4", - "createdAt": "2020-01-01T12:00:00Z", - "includesCreatedEdit": false, - "reactionGroups": [ - { - "content": "CONFUSED", - "users": { - "totalCount": 0 - } - }, - { - "content": "EYES", - "users": { - "totalCount": 0 - } - }, - { - "content": "HEART", - "users": { - "totalCount": 0 - } - }, - { - "content": "HOORAY", - "users": { - "totalCount": 0 - } - }, - { - "content": "LAUGH", - "users": { - "totalCount": 0 - } - }, - { - "content": "ROCKET", - "users": { - "totalCount": 0 - } - }, - { - "content": "THUMBS_DOWN", - "users": { - "totalCount": 0 - } - }, - { - "content": "THUMBS_UP", - "users": { - "totalCount": 0 - } - } - ] - }, - { - "author": { - "login": "marseilles" - }, - "authorAssociation": "COLLABORATOR", - "body": "Comment 5", - "createdAt": "2020-01-01T12:00:00Z", - "includesCreatedEdit": false, - "reactionGroups": [ - { - "content": "CONFUSED", - "users": { - "totalCount": 0 - } - }, - { - "content": "EYES", - "users": { - "totalCount": 0 - } - }, - { - "content": "HEART", - "users": { - "totalCount": 0 - } - }, - { - "content": "HOORAY", - "users": { - "totalCount": 0 - } - }, - { - "content": "LAUGH", - "users": { - "totalCount": 0 - } - }, - { - "content": "ROCKET", - "users": { - "totalCount": 0 - } - }, - { - "content": "THUMBS_DOWN", - "users": { - "totalCount": 0 - } - }, - { - "content": "THUMBS_UP", - "users": { - "totalCount": 0 - } - } - ] - } - ], - "totalCount": 5 - }, - "url": "https://github.com/OWNER/REPO/issues/123" - } - } - } -} diff --git a/pkg/cmd/issue/view/view.go b/pkg/cmd/issue/view/view.go index b1f4cba82..063be82c8 100644 --- a/pkg/cmd/issue/view/view.go +++ b/pkg/cmd/issue/view/view.go @@ -8,6 +8,7 @@ import ( "time" "github.com/MakeNowJust/heredoc" + "github.com/briandowns/spinner" "github.com/cli/cli/api" "github.com/cli/cli/internal/config" "github.com/cli/cli/internal/ghrepo" @@ -27,10 +28,9 @@ type ViewOptions struct { IO *iostreams.IOStreams BaseRepo func() (ghrepo.Interface, error) - SelectorArg string - WebMode bool - Comments int - CommentsProvided bool + SelectorArg string + WebMode bool + Comments bool } func NewCmdView(f *cmdutil.Factory, runF func(*ViewOptions) error) *cobra.Command { @@ -55,8 +55,6 @@ func NewCmdView(f *cmdutil.Factory, runF func(*ViewOptions) error) *cobra.Comman // support `-R, --repo` override opts.BaseRepo = f.BaseRepo - opts.CommentsProvided = cmd.Flags().Changed("comments") - if len(args) > 0 { opts.SelectorArg = args[0] } @@ -69,8 +67,7 @@ func NewCmdView(f *cmdutil.Factory, runF func(*ViewOptions) error) *cobra.Comman } cmd.Flags().BoolVarP(&opts.WebMode, "web", "w", false, "Open an issue in the browser") - cmd.Flags().IntVarP(&opts.Comments, "comments", "c", 1, "View issue comments sorted by most recent") - cmd.Flags().Lookup("comments").NoOptDefVal = "30" + cmd.Flags().BoolVarP(&opts.Comments, "comments", "c", false, "View issue comments") return cmd } @@ -82,20 +79,37 @@ func viewRun(opts *ViewOptions) error { } apiClient := api.NewClientFromHTTP(httpClient) - issue, _, err := issueShared.IssueWithCommentsFromArg(apiClient, opts.BaseRepo, opts.SelectorArg, opts.Comments) + issue, repo, err := issueShared.IssueFromArg(apiClient, opts.BaseRepo, opts.SelectorArg) if err != nil { return err } - openURL := issue.URL - if opts.WebMode { + openURL := issue.URL if opts.IO.IsStdoutTTY() { fmt.Fprintf(opts.IO.ErrOut, "Opening %s in your browser.\n", utils.DisplayURL(openURL)) } return utils.OpenInBrowser(openURL) } + if opts.Comments { + var s *spinner.Spinner + if opts.IO.IsStdoutTTY() { + s = utils.Spinner(opts.IO.ErrOut) + utils.StartSpinner(s) + } + + comments, err := api.CommentsForIssue(apiClient, repo, issue) + if err != nil { + return err + } + issue.Comments = *comments + + if opts.IO.IsStdoutTTY() { + utils.StopSpinner(s) + } + } + opts.IO.DetectTerminalTheme() err = opts.IO.StartPager() @@ -108,7 +122,7 @@ func viewRun(opts *ViewOptions) error { return printHumanIssuePreview(opts.IO, issue) } - if opts.CommentsProvided { + if opts.Comments { return printRawIssueComments(opts.IO.Out, issue) } @@ -144,7 +158,7 @@ func printRawIssueComments(out io.Writer, issue *api.Issue) error { return nil } -func rawIssueComment(comment api.IssueComment) string { +func rawIssueComment(comment api.Comment) string { var b strings.Builder fmt.Fprintf(&b, "author:\t%s\n", comment.Author.Login) fmt.Fprintf(&b, "association:\t%s\n", strings.ToLower(comment.AuthorAssociation)) @@ -172,7 +186,7 @@ func printHumanIssuePreview(io *iostreams.IOStreams, issue *api.Issue) error { ) // Reactions - if reactions := issue.ReactionGroups.String(); reactions != "" { + if reactions := reactionGroupList(issue.ReactionGroups); reactions != "" { fmt.Fprint(out, reactions) fmt.Fprintln(out) } @@ -210,7 +224,7 @@ func printHumanIssuePreview(io *iostreams.IOStreams, issue *api.Issue) error { // Comments if issue.Comments.TotalCount > 0 { - comments, err := issueComments(io, issue.Comments) + comments, err := issueCommentList(io, issue.Comments) if err != nil { return err } @@ -223,20 +237,20 @@ func printHumanIssuePreview(io *iostreams.IOStreams, issue *api.Issue) error { return nil } -func issueComments(io *iostreams.IOStreams, comments api.IssueComments) (string, error) { +func issueCommentList(io *iostreams.IOStreams, comments api.Comments) (string, error) { var b strings.Builder cs := io.ColorScheme() retrievedCount := len(comments.Nodes) hiddenCount := comments.TotalCount - retrievedCount if hiddenCount > 0 { - fmt.Fprint(&b, cs.Gray(fmt.Sprintf("———————— Hiding %v comments ————————", hiddenCount))) + fmt.Fprint(&b, cs.Gray(fmt.Sprintf("———————— Not showing %s ————————", utils.Pluralize(hiddenCount, "comment")))) fmt.Fprintf(&b, "\n\n\n") } for i, comment := range comments.Nodes { last := i+1 == retrievedCount - cmt, err := issueComment(io, comment, last) + cmt, err := formatIssueComment(io, comment, last) if err != nil { return "", err } @@ -254,7 +268,7 @@ func issueComments(io *iostreams.IOStreams, comments api.IssueComments) (string, return b.String(), nil } -func issueComment(io *iostreams.IOStreams, comment api.IssueComment, newest bool) (string, error) { +func formatIssueComment(io *iostreams.IOStreams, comment api.Comment, newest bool) (string, error) { var b strings.Builder cs := io.ColorScheme() @@ -274,7 +288,7 @@ func issueComment(io *iostreams.IOStreams, comment api.IssueComment, newest bool fmt.Fprintln(&b) // Reactions - if reactions := comment.ReactionGroups.String(); reactions != "" { + if reactions := reactionGroupList(comment.ReactionGroups); reactions != "" { fmt.Fprint(&b, reactions) fmt.Fprintln(&b) } @@ -334,3 +348,27 @@ func issueProjectList(issue api.Issue) string { } return list } + +func reactionGroupList(rgs api.ReactionGroups) string { + var rs []string + + for _, rg := range rgs { + if r := formatReactionGroup(rg); r != "" { + rs = append(rs, r) + } + } + + return strings.Join(rs, " • ") +} + +func formatReactionGroup(rg api.ReactionGroup) string { + c := rg.Count() + if c == 0 { + return "" + } + e := rg.Emoji() + if e == "" { + return "" + } + return fmt.Sprintf("%v %s", c, e) +} diff --git a/pkg/cmd/issue/view/view_test.go b/pkg/cmd/issue/view/view_test.go index 977e73dad..2b94c5a70 100644 --- a/pkg/cmd/issue/view/view_test.go +++ b/pkg/cmd/issue/view/view_test.go @@ -2,11 +2,13 @@ package view import ( "bytes" + "fmt" "io/ioutil" "net/http" "os/exec" "testing" + "github.com/briandowns/spinner" "github.com/cli/cli/internal/config" "github.com/cli/cli/internal/ghrepo" "github.com/cli/cli/internal/run" @@ -14,6 +16,7 @@ import ( "github.com/cli/cli/pkg/httpmock" "github.com/cli/cli/pkg/iostreams" "github.com/cli/cli/test" + "github.com/cli/cli/utils" "github.com/google/shlex" "github.com/stretchr/testify/assert" ) @@ -337,26 +340,31 @@ func TestIssueView_web_urlArg(t *testing.T) { func TestIssueView_tty_Comments(t *testing.T) { tests := map[string]struct { cli string - fixture string + fixtures map[string]string expectedOutputs []string wantsErr bool }{ "without comments flag": { - cli: "123", - fixture: "./fixtures/issueView_previewSingleComment.json", + cli: "123", + fixtures: map[string]string{ + "IssueByNumber": "./fixtures/issueView_previewSingleComment.json", + }, expectedOutputs: []string{ `some title`, `some body`, - `———————— Hiding 4 comments ————————`, + `———————— Not showing 4 comments ————————`, `marseilles \(collaborator\) • Jan 1, 2020 • Newest comment`, `Comment 5`, `Use --comments to view the full conversation`, `View this issue on GitHub: https://github.com/OWNER/REPO/issues/123`, }, }, - "with default comments flag": { - cli: "123 --comments", - fixture: "./fixtures/issueView_previewFullComments.json", + "with comments flag": { + cli: "123 --comments", + fixtures: map[string]string{ + "IssueByNumber": "./fixtures/issueView_previewSingleComment.json", + "CommentsForIssue": "./fixtures/issueView_previewFullComments.json", + }, expectedOutputs: []string{ `some title`, `some body`, @@ -374,35 +382,19 @@ func TestIssueView_tty_Comments(t *testing.T) { `View this issue on GitHub: https://github.com/OWNER/REPO/issues/123`, }, }, - "with specified comments flag": { - cli: "123 --comments=3", - fixture: "./fixtures/issueView_previewThreeComments.json", - expectedOutputs: []string{ - `some title`, - `some body`, - `———————— Hiding 2 comments ————————`, - `elvisp \(member\) • Jan 1, 2020`, - `Comment 3`, - `loislane \(owner\) • Jan 1, 2020`, - `Comment 4`, - `marseilles \(collaborator\) • Jan 1, 2020 • Newest comment`, - `Comment 5`, - `Use --comments to view the full conversation`, - `View this issue on GitHub: https://github.com/OWNER/REPO/issues/123`, - }, - }, - "with incorrect comments flag": { + "with invalid comments flag": { cli: "123 --comments 3", - fixture: "", wantsErr: true, }, } for name, tc := range tests { t.Run(name, func(t *testing.T) { + stubSpinner() http := &httpmock.Registry{} defer http.Verify(t) - if tc.fixture != "" { - http.Register(httpmock.GraphQL(`query IssueByNumber\b`), httpmock.FileResponse(tc.fixture)) + for name, file := range tc.fixtures { + name := fmt.Sprintf(`query %s\b`, name) + http.Register(httpmock.GraphQL(name), httpmock.FileResponse(file)) } output, err := runCommand(http, true, tc.cli) if tc.wantsErr { @@ -419,13 +411,15 @@ func TestIssueView_tty_Comments(t *testing.T) { func TestIssueView_nontty_Comments(t *testing.T) { tests := map[string]struct { cli string - fixture string + fixtures map[string]string expectedOutputs []string wantsErr bool }{ "without comments flag": { - cli: "123", - fixture: "./fixtures/issueView_previewSingleComment.json", + cli: "123", + fixtures: map[string]string{ + "IssueByNumber": "./fixtures/issueView_previewSingleComment.json", + }, expectedOutputs: []string{ `title:\tsome title`, `state:\tOPEN`, @@ -434,9 +428,12 @@ func TestIssueView_nontty_Comments(t *testing.T) { `some body`, }, }, - "with default comments flag": { - cli: "123 --comments", - fixture: "./fixtures/issueView_previewFullComments.json", + "with comments flag": { + cli: "123 --comments", + fixtures: map[string]string{ + "IssueByNumber": "./fixtures/issueView_previewSingleComment.json", + "CommentsForIssue": "./fixtures/issueView_previewFullComments.json", + }, expectedOutputs: []string{ `author:\tmonalisa`, `association:\t`, @@ -460,27 +457,8 @@ func TestIssueView_nontty_Comments(t *testing.T) { `Comment 5`, }, }, - "with specified comments flag": { - cli: "123 --comments=3", - fixture: "./fixtures/issueView_previewThreeComments.json", - expectedOutputs: []string{ - `author:\telvisp`, - `association:\tmember`, - `edited:\tfalse`, - `Comment 3`, - `author:\tloislane`, - `association:\towner`, - `edited:\tfalse`, - `Comment 4`, - `author:\tmarseilles`, - `association:\tcollaborator`, - `edited:\tfalse`, - `Comment 5`, - }, - }, - "with incorrect comments flag": { + "with invalid comments flag": { cli: "123 --comments 3", - fixture: "", wantsErr: true, }, } @@ -488,8 +466,9 @@ func TestIssueView_nontty_Comments(t *testing.T) { t.Run(name, func(t *testing.T) { http := &httpmock.Registry{} defer http.Verify(t) - if tc.fixture != "" { - http.Register(httpmock.GraphQL(`query IssueByNumber\b`), httpmock.FileResponse(tc.fixture)) + for name, file := range tc.fixtures { + name := fmt.Sprintf(`query %s\b`, name) + http.Register(httpmock.GraphQL(name), httpmock.FileResponse(file)) } output, err := runCommand(http, false, tc.cli) if tc.wantsErr { @@ -502,3 +481,8 @@ func TestIssueView_nontty_Comments(t *testing.T) { }) } } + +func stubSpinner() { + utils.StartSpinner = func(_ *spinner.Spinner) {} + utils.StopSpinner = func(_ *spinner.Spinner) {} +} From b2edf782cf205d158f8a300eeb730e2604ec5ebf Mon Sep 17 00:00:00 2001 From: Sam Coe Date: Tue, 8 Dec 2020 14:16:40 -0500 Subject: [PATCH 5/6] Reverse order of issue lookup checks --- pkg/cmd/issue/shared/lookup.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/pkg/cmd/issue/shared/lookup.go b/pkg/cmd/issue/shared/lookup.go index 675574f7c..6e716f6da 100644 --- a/pkg/cmd/issue/shared/lookup.go +++ b/pkg/cmd/issue/shared/lookup.go @@ -14,14 +14,6 @@ import ( func IssueFromArg(apiClient *api.Client, baseRepoFn func() (ghrepo.Interface, error), arg string) (*api.Issue, ghrepo.Interface, error) { issueNumber, baseRepo := issueMetadataFromURL(arg) - if baseRepo == nil { - var err error - baseRepo, err = baseRepoFn() - if err != nil { - return nil, nil, fmt.Errorf("could not determine base repo: %w", err) - } - } - if issueNumber == 0 { var err error issueNumber, err = strconv.Atoi(strings.TrimPrefix(arg, "#")) @@ -30,6 +22,14 @@ func IssueFromArg(apiClient *api.Client, baseRepoFn func() (ghrepo.Interface, er } } + if baseRepo == nil { + var err error + baseRepo, err = baseRepoFn() + if err != nil { + return nil, nil, fmt.Errorf("could not determine base repo: %w", err) + } + } + issue, err := issueFromNumber(apiClient, baseRepo, issueNumber) return issue, baseRepo, err } From efc05dee907cb764c659cf8f8a92d2c2652ce74d Mon Sep 17 00:00:00 2001 From: Sam Coe Date: Wed, 9 Dec 2020 13:50:08 -0500 Subject: [PATCH 6/6] Use spinner helper --- pkg/cmd/issue/view/view.go | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/pkg/cmd/issue/view/view.go b/pkg/cmd/issue/view/view.go index 063be82c8..d8754edcb 100644 --- a/pkg/cmd/issue/view/view.go +++ b/pkg/cmd/issue/view/view.go @@ -8,7 +8,6 @@ import ( "time" "github.com/MakeNowJust/heredoc" - "github.com/briandowns/spinner" "github.com/cli/cli/api" "github.com/cli/cli/internal/config" "github.com/cli/cli/internal/ghrepo" @@ -93,21 +92,13 @@ func viewRun(opts *ViewOptions) error { } if opts.Comments { - var s *spinner.Spinner - if opts.IO.IsStdoutTTY() { - s = utils.Spinner(opts.IO.ErrOut) - utils.StartSpinner(s) - } - + opts.IO.StartProgressIndicator() comments, err := api.CommentsForIssue(apiClient, repo, issue) + opts.IO.StopProgressIndicator() if err != nil { return err } issue.Comments = *comments - - if opts.IO.IsStdoutTTY() { - utils.StopSpinner(s) - } } opts.IO.DetectTerminalTheme()