Add support for response_plays #404
Conversation
There was a problem hiding this comment.
Thank you so much for the contribution. I've requested a few changes on this PR. Some of things I requested apply to multiple areas of this PR, but I only commented once to avoid repetition, so please let me know if you have any questions or if any of the feedback is unclear.
Also, could you please squash your commits into a single one? I'm cool with multiple commits if we're adding distinct features in each, but if the follow-up commits fix an earlier commit in the PR I'd rather we just squash them together to avoid the extra commits in the history.
|
@theckman thanks for your feedback! |
response_play.go
Outdated
| Runnability string `json:"runnability"` | ||
| ConferenceNumber string `json:"conference_number"` | ||
| ConferenceUrl string `json:"conference_url"` | ||
| ConferenceType string `json:"conference_type"` |
There was a problem hiding this comment.
So I think this PR is basically ready to go, but I do have some reservations about this struct (not anything you did) and would love to hear your thoughts on them.
Specifically, because all of these fields are string and not *string, consumers can't do a partial update of the play without potentially erasing data. They will need to set all of the fields on the struct, or those fields will be zeroed out when the API call is made if they aren't required.
A side effect of this is that consumers will almost always need to do a Get + Update operation, even if they are only updating a single field to a static value across all of their response plays.
Should we make these a *string and not a string, and include omitempty, so that folks can do partial updates?
I'm not necessarily asking that we do this, but am instead asking your opinion on it and what you'd maybe do if you were deciding.
There was a problem hiding this comment.
Thanks @theckman - yes I think that is a good suggestion, shall I make the changes and update the PR?
There was a problem hiding this comment.
Sorry for the delay, was noodling on this.
Yeah, let's go with changing these up to be pointer values. 👍
|
@petetanton thanks for this contribution. We're looking forward to use this feature :) |
|
I'll look at it shortly. Was focused on some of my own projects over the last few weeks. |
|
For some additional context, while I can merge the contributions of others into the |
Adding support for the response_plays API and enable adding response_plays to services