Request parent team from endpoints that return a team #718
Conversation
There was a problem hiding this comment.
Thank you, @e-beach!
For each test function that tests one of the methods where you added the new mediaTypeNestedTeamsPreview header, could you please also add a testHeader call in order to be consistent with the rest of the package?
Otherwise, these changes LGTM.
There was a problem hiding this comment.
Upon a closer look, I may have to backtrack on something I agreed with earlier in #714 (comment).
I've realized that #714 is not about adding an entirely new API, but making modifications to an existing API.
That means that implementing it piece-by-piece may leave the master branch of go-github in an unusable state. The behavior will not be consistent with previous API (without preview API content-type header), nor with the new preview API (with preview API content-type header). It'll be something weird and hard to understand in between.
For instance, consider this bullet point from the change announcement:
- Endpoints that return team hashes will now include the parent field representing the parent team. Additionally, the
members_countandrepositories_countwill include all members and repositories according to the nested structure.
Those fields are a part of the Team struct. The endpoints that have the application/vnd.github.hellcat-preview+json header set will return different numbers than other endpoints where application/vnd.github.hellcat-preview+json isn't yet set.
Does that make sense? Do you agree that partially implementing #714 at a time is not a viable approach?
However, I still think keeping PRs small will be helpful; I'd rather not make it so that implementing this feature has to be done all by one person all in one huge PR. I have an alternative idea.
We could create a feature branch and send PRs into that feature branch. Once it has all of #714 implemented, we can merge it into master, therefore implementing #714 on master atomically rather than piece by piece. (Of course, a separate feature branch has downsides of its own, but it seems to be the least evil solution to this challenge. I'm open to feedback and suggestions.) /cc @gmlewis
If you agree, I think we should proceed by closing this PR, creating a feature branch, then asking @e-beach to re-send this PR into that feature branch rather than master.
| testHeader(t, r, "Accept", mediaTypeNestedTeamsPreview) | ||
| fmt.Fprint(w, `{"id":1, "name":"n", "description": "d", "url":"u", "slug": "s", "permission":"p", | ||
| "parent": {"id":2, "name":"n", "description": "d", "parent": null}}`) | ||
|
|
There was a problem hiding this comment.
Minor, unneeded blank line here.
|
Moving the changes to a separate branch sounds logical to me - it doesn't make sense to add potentially breaking changes piecemeal. The impact of a change such as listing members of child teams, instead of just the parent team, should be handled carefully. A feature branch seems the correct way to group a set of PRs that should be merged collectively. I guess we are awaiting @gmlewis's opinion, then. |
googlebot
added
the
cla: yes
|
A separate feature branch seems totally reasonable to me for this use case. I say "go for it". |
|
Okay. I've created a |
|
I just discovered that it is possible to edit a PR to be against a different branch. |
|
Oh, my bad, I forgot that it's possible now. All right. |
This PR address part of #714 by
parentfield to theTeamstruct.Initially, I had added the preview header to the
ListTeamMembers,IsTeamMember, andGetTeamMembershipendpoints, but dropped those changes because they aren't relevant to the goal of this PR.