Allow an empty array of BypassActors in Ruleset struct in CreateRuleset endpoint #3174
Conversation
Co-authored-by: Daniel Liao <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3174 +/- ##
==========================================
- Coverage 97.72% 92.95% -4.77%
==========================================
Files 153 171 +18
Lines 13390 11566 -1824
==========================================
- Hits 13085 10751 -2334
- Misses 215 724 +509
- Partials 90 91 +1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
It think it might be a more pleasant user experience to not introduce a new struct,
but just a new endpoint that still uses the Ruleset struct as input, but internally uses a non-exported struct like the one you created.
To do this, you would need to copy over the fields to this non-exported struct and then send that instead of the original Ruleset.
Also, let's please not start using BPActr as a short abbreviation for BypassActor, as we try to make this repo as searchable as possible, and nobody is going to guess bpactr as a search string.
Also, all new godoc-style comments should be complete sentences with ending periods. Thanks.
|
Do you mean something like having an anonymous struct identical to Ruleset in the new functions scope that would just copy over the values of Ruleset to? I understand your idea but i'm confused about the implementation. |
Yes, that is what I mean. We have other examples of this in our repo. Here is one of many: https://github.com/google/go-github/blob/master/github/repos.go#L546-L573 I found this by typing: $ grep 'type [a-z]' github/*.go |
|
made some changes. Let me know if its what you expected. |
Co-authored-by: Glenn Lewis <[email protected]>
Co-authored-by: Glenn Lewis <[email protected]>
|
Ok. I commited your suggested changes. I didn't know rulesetNoOmitBypassActors made it non exported. Learned something new about the language. Thanks! |
Co-authored-by: Glenn Lewis <[email protected]>
Co-authored-by: Glenn Lewis <[email protected]>
|
Please re-run step 4 in CONTRIBUTING.md to re-generate the auto-generated files... then the tests should all pass. |
…oBypassActor, regenerated files
There was a problem hiding this comment.
Thank you, @Matthew-Reidy !
LGTM.
Awaiting second LGTM+Approval from any other contributor to this repo before merging.
| @@ -395,6 +395,24 @@ type Ruleset struct { | |||
| Rules []*RepositoryRule `json:"rules,omitempty"` | |||
| } | |||
|
|
|||
| // rulesetNoOmitBypassActors represents a GitHub ruleset object. The struct does not omit bypassActors if the field is nil or an empty array is passed. | |||
| type rulesetNoOmitBypassActors struct { | |||
There was a problem hiding this comment.
I think that you can embed the entire Ruleset object and shadow just the bypass_actors
type rulesetNoOmitBypassActors struct {
Ruleset
BypassActors []*BypassActor `json:"bypass_actors"`
}example: https://goplay.tools/snippet/7lHZci7LW0y
@gmlewis WDYT?
Then you can use it like so:
func (s *RepositoriesService) UpdateRulesetNoBypassActor(...rs *Ruleset...
rsNoBypassActor := &rulesetNoOmitBypassActors{}
if rs != nil {
rsNoBypassActor := &rulesetNoOmitBypassActors{Ruleset: *rs}
}There was a problem hiding this comment.
Sounds great, @tomfeigin - as long as we have a unit test that demonstrates this behavior (meaning a BypassActors=nil still gets passed through in JSON as "bypass_actors":[].
| @@ -507,6 +525,48 @@ func (s *RepositoriesService) UpdateRuleset(ctx context.Context, owner, repo str | |||
| return ruleset, resp, nil | |||
| } | |||
|
|
|||
| // UpdateRulesetNoBypassActor updates a ruleset for the specified repository. | |||
| // | |||
| // This function is necessary as the UpdateRuleset function does not marschall ByPassActor if passed as nil or an empty array. | |||
There was a problem hiding this comment.
NIT: marschall -> marshal
Fixes: #3137
added an extra Ruleset struct and CreateRuleset endpoint in repo_rules.go to allow the BypassActors field to be passed as nil or an empty array