Skip to content

Add gh issue close <urlOrNumber>#843

Merged
probablycorey merged 14 commits into
masterfrom
when-god-closes-an-issue-he-opens-a-pull-request
Apr 29, 2020
Merged

Add gh issue close <urlOrNumber>#843
probablycorey merged 14 commits into
masterfrom
when-god-closes-an-issue-he-opens-a-pull-request

Conversation

@probablycorey

@probablycorey probablycorey commented Apr 28, 2020

Copy link
Copy Markdown
Contributor

This implements gh issue close <issue number or url>. If it is already closed it outputs a warning. If the repo doesn't allow issues, then it results in an error.

Below is an AMAZING screenshot of it it in action!

#413

probablycorey and others added 6 commits April 28, 2020 09:11
Add test for closing issue

Co-Authored-By: Nate Smith <vilmibm@neongrid.space>
Co-Authored-By: Nate Smith <vilmibm@neongrid.space>
Co-Authored-By: Nate Smith <vilmibm@neongrid.space>
Co-Authored-By: Nate Smith <vilmibm@neongrid.space>
Co-Authored-By: Nate Smith <vilmibm@neongrid.space>
@probablycorey probablycorey self-assigned this Apr 28, 2020
@ampinsk

ampinsk commented Apr 28, 2020

Copy link
Copy Markdown

Woo this looks great! 2 thoughts:

  1. Any thoughts on making the check red? I know it might be weird for a success message but I feel like it'd be nice to align with the "closed" color you expect
  2. Could we make these sentence case?

Screen Shot 2020-04-28 at 12 22 21 PM

@probablycorey

Copy link
Copy Markdown
Contributor Author

@ampinsk Made those changes. The red checkmark both seems correct and incorrect, but so does the green checkmark. So I say lets try it out.

@probablycorey probablycorey marked this pull request as ready for review April 28, 2020 20:08

@vilmibm vilmibm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 i'm just being nitpicky about testing

Comment thread command/issue_test.go
if !strings.Contains(err.Error(), "issues disabled") {
t.Fatalf("got unexpected error: %s", err)
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe a third test for the already closed state

Comment thread test/fixtures/issueClose.json Outdated
@@ -0,0 +1 @@
{ "id": "hi" }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think it would be fine to inline this? it's tiny and only used once. you can do:

http.StubResponse(200, bytes.NewBufferString(`{"id": "hi"}`))

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good point...

@probablycorey

Copy link
Copy Markdown
Contributor Author

@vilmibm I've got the changes you requested in

@vilmibm vilmibm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hot damn and hell yeah

@probablycorey probablycorey merged commit 28b35aa into master Apr 29, 2020
@probablycorey probablycorey deleted the when-god-closes-an-issue-he-opens-a-pull-request branch April 29, 2020 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants