-
Notifications
You must be signed in to change notification settings - Fork 5.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
gh status
: show status even if a comment 404s
#7873
Merged
Merged
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -295,6 +295,63 @@ func TestStatusRun(t *testing.T) { | |
warning: You don't have permission to access this resource. | ||
`), | ||
}, | ||
{ | ||
name: "not found errors", | ||
httpStubs: func(reg *httpmock.Registry) { | ||
reg.Register( | ||
httpmock.GraphQL("UserCurrent"), | ||
httpmock.StringResponse(`{"data": {"viewer": {"login": "jillvalentine"}}}`)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I enjoy Resident Evil, too 🧟 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤷 Didn't catch the reference, actually! Just based on the surrounding stubs. |
||
reg.Register( | ||
httpmock.REST("GET", "repos/rpd/todo/issues/110"), | ||
httpmock.StringResponse(`{"body":"hello @jillvalentine how are you"}`)) | ||
reg.Register( | ||
httpmock.REST("GET", "repos/rpd/todo/issues/4113"), | ||
httpmock.StringResponse(`{"body":"this is a comment"}`)) | ||
reg.Register( | ||
httpmock.REST("GET", "repos/cli/cli/issues/1096"), | ||
httpmock.StringResponse(`{"body":"@jillvalentine hi"}`)) | ||
reg.Register( | ||
httpmock.REST("GET", "repos/rpd/todo/issues/comments/1065"), | ||
httpmock.StatusStringResponse(404, `{ | ||
"message": "Not Found." | ||
}`)) | ||
reg.Register( | ||
httpmock.REST("GET", "repos/vilmibm/gh-screensaver/issues/comments/10"), | ||
httpmock.StringResponse(`{"body":"a message for @jillvalentine"}`)) | ||
reg.Register( | ||
httpmock.GraphQL("AssignedSearch"), | ||
httpmock.FileResponse("./fixtures/search.json")) | ||
reg.Register( | ||
httpmock.REST("GET", "notifications"), | ||
httpmock.FileResponse("./fixtures/notifications.json")) | ||
reg.Register( | ||
httpmock.REST("GET", "users/jillvalentine/received_events"), | ||
httpmock.FileResponse("./fixtures/events.json")) | ||
}, | ||
opts: &StatusOptions{}, | ||
wantOut: heredoc.Doc(` | ||
Assigned Issues │ Assigned Pull Requests | ||
vilmibm/testing#157 yolo │ cli/cli#5272 Pin extensions | ||
cli/cli#3223 Repo garden...│ rpd/todo#73 Board up RPD windows | ||
rpd/todo#514 Reducing zo...│ cli/cli#4768 Issue Frecency | ||
vilmibm/testing#74 welp │ | ||
adreyer/arkestrator#22 complete mo...│ | ||
│ | ||
Review Requests │ Mentions | ||
cli/cli#5272 Pin extensions │ rpd/todo#110 hello @j... | ||
vilmibm/testing#1234 Foobar │ cli/cli#1096 @jillval... | ||
rpd/todo#50 Welcome party...│ rpd/todo#116 (could n... | ||
cli/cli#4671 This pull req...│ vilmibm/gh-screensaver#15 a messag... | ||
rpd/todo#49 Haircut for Leon│ | ||
│ | ||
Repository Activity | ||
rpd/todo#5326 new PR Only write UTF-8 BOM on W... | ||
vilmibm/testing#5325 comment on Ability to sea... We are working on dedicat... | ||
cli/cli#5319 comment on [Codespaces] D... Wondering if we shouldn't... | ||
cli/cli#5300 new issue Terminal bell when a runn... | ||
|
||
`), | ||
}, | ||
{ | ||
name: "notification errors", | ||
httpStubs: func(reg *httpmock.Registry) { | ||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we actually want to display anything to the user in this case? They have no actions they can take to view the comment so this message does not provide a lot of value in my opinion. I would rather see us just skip to the next one and not display anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also waffled on that. Personally, I prefer seeing that a notification exists even if I can't see the text. The comment URL still shows up, which gives some context, e.g. indicating there's a comment a former client meant to be seen. (I suspect that they only exist for comments made before access was restricted, not new comments tagged privately…but I don't know). In that case, the action might be to reach out to the client and let them know they might need to fill you in directly.
🤷 But I also don't feel too strongly about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the more common case of this occurring is that a comment was purposefully deleted for one reason or another and in that case showing nothing seems like the correct behavior.
For now lets go with not displaying anything and we can change the behavior in the future if we hear from users that they would rather have the other behavior.