-
Notifications
You must be signed in to change notification settings - Fork 2.2k
feat: Document review request events in Timeline struct
#3391
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3391 +/- ##
==========================================
- Coverage 97.72% 92.21% -5.51%
==========================================
Files 153 173 +20
Lines 13390 14770 +1380
==========================================
+ Hits 13085 13620 +535
- Misses 215 1060 +845
Partials 90 90 ☔ View full report in Codecov by Sentry. |
f46f7be to
4a77129
Compare
review_requested and review_request_removed to event types as comment in issues_timeline.go
review_requested and review_request_removed to event types as comment in issues_timeline.goTimeline struct
7986ead to
041b2dd
Compare
|
I've run all the scripts in step 4 of |
041b2dd to
da5b558
Compare
da5b558 to
2647563
Compare
Timeline structTimeline struct
gmlewis
left a comment
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.
Thank you, @jbrill !
github/issues_timeline_test.go
Outdated
| "github.com/google/go-cmp/cmp" | ||
| ) | ||
|
|
||
| func parseTime(t *testing.T, value string) time.Time { |
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.
Honestly, there is really no need for this helper function, as there is no need to test the functionality of time.Parse in this repo since it is well-tested with each release of the Go compiler.
The vast majority of our tests use &Timestamp{referenceTime} although I do see some unfortunate deviations from this.
Let's please remove this helper function and use the existing referenceTime instead.
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.
Sounds good. I've pushed up those changes to use &Timestamp{referenceTime}!
|
Oh, and as described in CONTRIBUTING.md, please do not use force-push in this repo as we always squash-and-merge to create a clean commit history. |
Apologies -- pushed up a new commit with the test changes. |
gmlewis
left a comment
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.
Thank you, @jbrill !
LGTM.
Merging.
Add review request event documentation to Timeline struct
Updates the Timeline struct documentation to include review request-related events that were missing from the Event field description, matching the GitHub API behavior.
Changes
review_requestedeventRequesterorRequestedTeamwill be populatedReviewerwill be populatedreview_request_removedeventReviewerwill be populatedRequesterorRequestedTeamwill be populatedThese events were already supported in the implementation via the corresponding struct fields, but were missing from the Event field's documentation.
Local Testing
Tests Written
TestTimeline_ReviewRequestsreview_requestedandreview_request_removedevents.RequesterandReviewerfields are correctly populated.Timelinestruct accurately reflects the GitHub API's behavior for these events.