Show the state (open, closed, merged) in issue view and pr view by doi-t · Pull Request #667 · cli/cli

doi-t · 2020-03-17T23:41:45Z

Closes #652

Mostly tests for each state. Changes for the core logics are a few lines in command/issue.go, command/pr.go and GraphQL queries.

Since colors matter, I tried to test PR/Issue states with ansi escape sequence to compare colors along with its string but couldn't figure out an efficient way to do... I might be missing something for it.

Test cases for `gh [pr|issue] view --preview`

I use • (Bullet: U+2022) as a separator. It is a bit ugly in below captures but I think it is my terminal font problem. 💦

See cli#652 (comment)

doi-t · 2020-03-17T23:46:35Z

utils/utils.go

 }
+
+// ColorFuncForState returns a color function for a PR/Issue state
+func ColorFuncForState(state string) func(string) string {


Since colorFuncForState was referenced from both command/issue.go and command/pr.go, I moved it to utils.go.

Nit: could you move this back to the command package, since it's only needed there? We're trying to slim down the utils package until we can remove it.

I can live in either issue.go or pr.go; it doesn't matter.

ampinsk · 2020-03-18T00:35:55Z

(idea) We could change Open to Draft like below if it is more appropriate

I like this idea! This would match better with github.com:

doi-t · 2020-03-18T01:10:29Z

Updated code to show Draft instead of Open for a Draft PR!

Comfirmed that it works for both PR by number and PR for a branch name.

doi-t · 2020-03-19T03:03:48Z

It seems that this PR will have further conflicts with #665. I agree with the breaking backwards compatibility! So (maybe?) I should wait for merging #665 and resolve conflicts here. 🤔

doi-t · 2020-03-25T15:14:30Z

@mislav @vilmibm
Hi! It seems that issues for stability and the default behavior are prioritized these days so that I assume that this PR is not in the queue for review yet. But let me make sure your thoughts especially for testing at least.
Is it ok to make another PR for #663 based on this branch (doi-t:view-the-current-state)? In this PR, I introduced table-driven testing of PR/Issue previews to test several PR/Issue states (Closed, Merged, Draft) and I would like to expand it more in another PR to test PR/Issue metadata because it allows me to avoid repeated codes and easily add more tests for the preview outputs (I even think that it could eventually be a shared helper function for other test cases 🤔).
Of course, I will take care of any conflicts with the upstream for it. Hope it is not too different from your plan. Please let me know if there is anything that I can make things better. 🙏

mislav · 2020-03-26T14:17:16Z

Is it ok to make another PR for #663 based on this branch

Of course! That sounds reasonable. Please feel free to make a nested PR 👍

mislav

This looks great to me. Thanks for the hard work @doi-t! ✨

mislav · 2020-03-31T15:26:48Z

command/issue_test.go

+	for name, tc := range tests {
+		t.Run(name, func(t *testing.T) {
+			got := issueStateTitleWithColor(tc.state)
+			diff := cmp.Diff(tc.want, got)


TIL go-cmp! 🏆

mislav · 2020-03-31T15:28:04Z

command/issue_test.go

-	http := initFakeHTTP()
-	http.StubRepoResponse("OWNER", "REPO")
+	for name, tc := range tests {
+		t.Run(name, func(t *testing.T) {


Massive props for table-driven test 🥇

mislav · 2020-03-31T15:32:42Z

utils/utils.go

 }
+
+// ColorFuncForState returns a color function for a PR/Issue state
+func ColorFuncForState(state string) func(string) string {


Nit: could you move this back to the command package, since it's only needed there? We're trying to slim down the utils package until we can remove it.

I can live in either issue.go or pr.go; it doesn't matter.

doi-t · 2020-04-01T04:05:46Z

@mislav
Thank you for the review! I need to solve the conflicts to reflect changes of #665. Please wait a moment before the merge. I will poke you again if I make a notable change through solving the conflicts.

doi-t · 2020-04-02T15:45:48Z

@mislav
I've solved the conflicts. I've also rolled back colorFuncForState 4c2f15f and simplified tests with the new testing helper test.ExpectLines 7eabbf5.

I believe it is ready for merge now if it looks good to you!

mislav

This looks excellent to me. Thank you for the hard work 🌟

doi-t added 16 commits March 16, 2020 02:25

Add 'state' and 'createdAt' to the query for IssueByNumber

94a1e6a

Add 'state' to queries for pull requests

d429bab

Move colorFuncForState for a PR/issue state to the utils package

faa4bed

Show the issue state for viewing issues in CLI

caf26e4

Show the PR state for viewing PRs in CLI

b8edb9f

Fix tests for preview messages with a issue/PR state

e0bfd67

Extract test fixtures for 'issue view --preview'

0475cf0

Add a test for the closed issue preview

8cd6932

Add tests for the Closed/Merged PR preview

5a23113

Add a test for viewing an issue without labels in CLI

c8a72c3

Format PR/Issue states with color

9a6026e

Remove labels from the issue preview header

e85259d

See cli#652 (comment)

Fix utility function scopes

46a632c

Add tests for utilities of PR/issue state format

0cfa4be

Add a missing isDraft for querying PR by number

7b5a0b5

Add tests for viewing a Draft PR in CLI

762e806

doi-t commented Mar 17, 2020

View reviewed changes

doi-t marked this pull request as ready for review March 17, 2020 23:49

doi-t added 2 commits March 18, 2020 08:51

Merge branch 'master' into view-the-current-state

7ceffd0

Add test fixtures for viewing Draft state in CLI

cf69d7e

doi-t mentioned this pull request Mar 18, 2020

Viewing issues and PRs in CLI should show the state (open, closed, merged) #652

Closed

doi-t added 3 commits March 18, 2020 09:55

Rename OPEN state to Draft for a Draft PR

e50ba54

Fix tests to support the "Draft" state badge

327a804

Cleanup

a5bd313

doi-t added 2 commits March 20, 2020 16:32

Apply table driven testing for pr/issue preview commands

1a5e9f1

Merge branch 'master' into view-the-current-state

0ba0a07

doi-t force-pushed the view-the-current-state branch from 7509efa to 0ba0a07 Compare March 20, 2020 09:17

Merge branch 'master' into view-the-current-state

7d42f46

mislav approved these changes Mar 31, 2020

View reviewed changes

doi-t added 3 commits April 2, 2020 23:52

Merge branch 'master' into view-the-current-state

0095fe9

Roll back the place of colorFuncForState

4c2f15f

Simplify expected output definitions with new line testing helper

7eabbf5

mislav approved these changes Apr 2, 2020

View reviewed changes

doi-t changed the title ~~Viewing issues and PRs in CLI should show the state (open, closed, merged)~~ Show the state (open, closed, merged) in issue view and pr view Apr 3, 2020

doi-t mentioned this pull request Apr 3, 2020

Add relevant metadata to issue view in CLI #745

Merged

mislav merged commit e10ccef into cli:master Apr 3, 2020

doi-t deleted the view-the-current-state branch April 4, 2020 04:43

doi-t mentioned this pull request Apr 4, 2020

Add relevant metadata to issue view and pr view #663

Closed

Conversation

doi-t commented Mar 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test cases for gh [pr|issue] view --preview

Uh oh!

doi-t Mar 17, 2020 • 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!

mislav Mar 31, 2020

Choose a reason for hiding this comment

Uh oh!

ampinsk commented Mar 18, 2020

Uh oh!

doi-t commented Mar 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

doi-t commented Mar 19, 2020

Uh oh!

doi-t commented Mar 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mislav commented Mar 26, 2020

Uh oh!

mislav left a comment

Choose a reason for hiding this comment

Uh oh!

mislav Mar 31, 2020

Choose a reason for hiding this comment

Uh oh!

mislav Mar 31, 2020

Choose a reason for hiding this comment

Uh oh!

mislav Mar 31, 2020

Choose a reason for hiding this comment

Uh oh!

doi-t commented Apr 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

doi-t commented Apr 2, 2020

Uh oh!

mislav left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

doi-t commented Mar 17, 2020 •

edited

Loading

Test cases for `gh [pr|issue] view --preview`

doi-t Mar 17, 2020 •

edited

Loading

doi-t commented Mar 18, 2020 •

edited

Loading

doi-t commented Mar 25, 2020 •

edited

Loading

doi-t commented Apr 1, 2020 •

edited

Loading