-
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
Add referenced workflows to WorkflowRun #2975
Conversation
Path *string `json:"path,omitempty"` | ||
SHA *string `json:"sha,omitempty"` |
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.
path and sha are required in the response schema, so they can be string
instead of *string
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.
Just FYI - overall, we've been very lax on the "receive-side" of responses from the GitHub server over the years in terms of what the official docs say will definitely be returned versus what optionally might be returned.
One of the reasons we've done this is first, it has not always been clear from the documentation what will or will not be sent back, and we've also discovered experientially that there are sometimes surprises depending on where a particular struct is embedded (for example, if we were to add this to a different method's return struct).
Additionally, years back we added the helper methods to get field values that are pointers which help tremendously to avoid panics if something was expected but didn't show up in the response.
So I believe our official stance is to try and be super careful about the omitempty
on the request side of the usage, but on the response side, we are happy to err on the side of more fields with pointers (rather than fewer).
So if @WillAbides had not mentioned this, I would have allowed this to go through, however I'm fine also with making the change since the official docs are hopefully correct and will not change soon.
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.
Thanks. That's good to read. I didn't realize the distinction between request and response fields.
I'm going to change my review to ✅ as soon as I hit submit on this comment. Then I might make a PR adding a note about this to CONTRIBUTING.md.
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.
So I wrote "official stance" above and that was probably too strongly worded 😄 .
The above is my "unofficial understanding" of how we are guiding the maintenance of this repo based upon years of observation.
The final authority is the original author of this repo, Will Norris, a friend and former coworker of mine, who asked me many years ago to help maintain this repo. I'm sure he listens in, as I hear from him every now and then, but I hate to ping him unless absolutely necessary. 😄
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.
@gmlewis thanks for the walkthrough in the past :) I'm happy to make any changes necessary. The reason I made them pointers is just to follow a patten with other required fields in WorkflowRun
.
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.
Looks good except for changing the types to non-pointer.
Codecov Report
@@ Coverage Diff @@
## master #2975 +/- ##
=======================================
Coverage 98.11% 98.11%
=======================================
Files 149 149
Lines 12796 12796
=======================================
Hits 12555 12555
Misses 166 166
Partials 75 75
|
OK, so if we are leaving the structs as is, which is fine with me, then you will need to run |
9c61789
to
435c2bd
Compare
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, @schaffe-cb and @WillAbides !
LGTM.
Merging.
Closes #2974