-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Document required branch protection fields. #519
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
An alternative solution to consider is for us to automatically convert I suspect doing that could be slightly better, but I'm also okay with the current solution if others think it's fine. |
fdb4af2
to
10eb3f4
Compare
I think that documenting this is excellent. If someone would like to automatically convert the nil slices to non-nil empty slices and remove the comments, that might make for a nice starter project and I would approve that too. |
All of those fields are documented as required at https://developer.github.com/v3/repos/branches/#update-branch-protection. In #512, we changed the API to better reflect that. However, it should still be documented that the fields are required. This is consistent with the other required fields that are also documented as required. Additionally, note that empty slices must be non-nil. From running integration tests, I've seen that passing nil slices (which get encoded as null via encoding/json) resulted in invalid request errors such as: No subschema in "anyOf" matched. For 'properties/users', nil is not an array. For 'properties/teams', nil is not an array. Not all subschemas of "allOf" matched. For 'anyOf/1', {"users"=>nil, "teams"=>nil} is not a null. [] Follows #512.
10eb3f4
to
832a08c
Compare
Sounds good. I'll leave this for a few days in case @alindeman would like to leave any comments, then merge. |
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.
LGTM. Can't argue with better docs. Seems clear and practical to me.
All of those fields are documented as required at https://developer.github.com/v3/repos/branches/#update-branch-protection. In google#512, we changed the API to better reflect that. However, it should still be documented that the fields are required. This is consistent with the other required fields that are also documented as required. Additionally, note that empty slices must be non-nil. From running integration tests, I've seen that passing nil slices (which get encoded as null via encoding/json) resulted in invalid request errors such as: No subschema in "anyOf" matched. For 'properties/users', nil is not an array. For 'properties/teams', nil is not an array. Not all subschemas of "allOf" matched. For 'anyOf/1', {"users"=>nil, "teams"=>nil} is not a null. [] Follows google#512.
All of those fields are documented as required at https://developer.github.com/v3/repos/branches/#update-branch-protection.
In #512, we changed the API to better reflect that. However, it should still be documented that the fields are required. This is consistent with the other required fields that are also documented as required.
Additionally, note that empty slices must be non-
nil
. From running integration tests, I've seen that passingnil
slices (which get encoded asnull
viaencoding/json
) resulted in invalid request errors such as:Follows #512.
/cc @alindeman since you worked on #512 where we discussed this.