Document required branch protection fields.#519
Merged
Conversation
Member
Author
|
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
Collaborator
|
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
Member
Author
|
Sounds good. I'll leave this for a few days in case @alindeman would like to leave any comments, then merge. |
alindeman
approved these changes
Jan 18, 2017
Contributor
alindeman
left a comment
There was a problem hiding this comment.
LGTM. Can't argue with better docs. Seems clear and practical to me.
bubg-dev
pushed a commit
to bubg-dev/go-github
that referenced
this pull request
Jun 16, 2017
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 passingnilslices (which get encoded asnullviaencoding/json) resulted in invalid request errors such as:Follows #512.
/cc @alindeman since you worked on #512 where we discussed this.