-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
feat: Document review request events in Timeline
struct
#3391
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.go
Timeline
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
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. |
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_requested
eventRequester
orRequestedTeam
will be populatedReviewer
will be populatedreview_request_removed
eventReviewer
will be populatedRequester
orRequestedTeam
will 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_ReviewRequests
review_requested
andreview_request_removed
events.Requester
andReviewer
fields are correctly populated.Timeline
struct accurately reflects the GitHub API's behavior for these events.