Conversation
9daa214 to
609b3e6
Compare
mislav
left a comment
There was a problem hiding this comment.
This looks great! Love the use of Comment interface.
My asks are related to ironing out some functionality, such as handling of review statuses PENDING and DISMISSED.
| ) | ||
|
|
||
| func RawCommentList(comments api.Comments) string { | ||
| type Comment interface { |
|
@samcoe Something that I just noticed: until we have support for displaying nested comments within a review, maybe we should skip rendering COMMENTED reviews that don't have their own body. Because of our back-and-forth right now, this
|
|
@mislav I agree, and updated the PR to skip rendering |
mislav
left a comment
There was a problem hiding this comment.
Updates look good! Only polish and possible loading optimization remains.
| count := 1 | ||
| go func() { | ||
| reviews, err := api.ReviewsForPullRequest(apiClient, repo, pr) | ||
| pr.Reviews = *reviews |
There was a problem hiding this comment.
Is this a safe operation to do in the goroutine? Or should reviews be passed back to the main thread via a channel and assigned there? Same question for comments below.
There was a problem hiding this comment.
In Go, we will often hear the mantra: “don't communicate by sharing memory; share memory by communicating”.
Even though your current approach should work, if we follow the above mantra, it would be generally better to return the reviews and comments from goroutines via channels and assign them to the PR in the main goroutine.
However, I can't envision a case where your current code fails, and it's relatively clean, so I would also be fine if you keep it as-is.
pkg/cmd/pr/view/view.go
Outdated
| for i := 0; i < count; i++ { | ||
| if e := <-errc; e != nil { | ||
| err = e | ||
| } | ||
| } |
There was a problem hiding this comment.
This could be replicated using sync.WaitGroup.
var wg sync.WaitGroup
wg.Add(1)
go func() {
defer wg.Done()
var reviews *api.PullRequestReviews
reviews, err = api.ReviewsForPullRequest(apiClient, repo, pr)
pr.Reviews = *reviews
}()
if opts.Comments {
wg.Add(1)
go func() {
defer wg.Done()
var comments *api.Comments
comments, err = api.CommentsForPullRequest(apiClient, repo, pr)
pr.Comments = *comments
}()
}
wg.Wait()
return pr, errAre there performance implications or other downsides to this implementation? Seems like using the built in WaitGroup might be better.
There was a problem hiding this comment.
Good call! WaitGroup is more idiomatic, so I would definitely use it here.
mislav
left a comment
There was a problem hiding this comment.
Looks great! Thanks for the updates ✨
pkg/cmd/pr/view/view.go
Outdated
| for i := 0; i < count; i++ { | ||
| if e := <-errc; e != nil { | ||
| err = e | ||
| } | ||
| } |
There was a problem hiding this comment.
Good call! WaitGroup is more idiomatic, so I would definitely use it here.
| count := 1 | ||
| go func() { | ||
| reviews, err := api.ReviewsForPullRequest(apiClient, repo, pr) | ||
| pr.Reviews = *reviews |
There was a problem hiding this comment.
In Go, we will often hear the mantra: “don't communicate by sharing memory; share memory by communicating”.
Even though your current approach should work, if we follow the above mantra, it would be generally better to return the reviews and comments from goroutines via channels and assign them to the PR in the main goroutine.
However, I can't envision a case where your current code fails, and it's relatively clean, so I would also be fine if you keep it as-is.
7dbf28e to
6fbc5e6
Compare
6fbc5e6 to
b9b1079
Compare
This PR adds rendering of top level pull request review comments to
pr view. They are displayed inline with regular comments the same way asGitHub.com.cc #1009