Skip to content

parent_team_id is an integer identifier, not a string #748

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

Merged
merged 1 commit into from
Oct 13, 2017

Conversation

alindeman
Copy link
Contributor

@alindeman alindeman commented Oct 10, 2017

Using the code as written in the NestedTeamsPreview branch produces an error when talking to the GitHub API:

panic: PATCH https://github.com/api/teams/2514825: 422 Invalid request.

For 'properties/parent_team_id', "2514824" is not a number or null. []

The documentation states that this parameter is an id type which--for the GitHub API--appears to mean an integer, not a string.

@googlebot googlebot added the cla: yes Indication that the PR author has signed a Google Contributor License Agreement. label Oct 10, 2017
@alindeman
Copy link
Contributor Author

👀 @elliott-beach @gmlewis

@elliott-beach
Copy link
Contributor

elliott-beach commented Oct 10, 2017

LGTM,

But I think GitHub should edit their docs. This may be the first JSON parameter in the API that has the type of id. When API responses contain ids, the type is an integer, not a string.

It seems the documentation is ambiguous on the argument type - I have emailed GitHub.

@elliott-beach
Copy link
Contributor

elliott-beach commented Oct 12, 2017

Response from GitHub:

I see what you mean now and agree it would be helpful to update the type there. I've opened an issue internally for our Documentation team to take a look and consider updating for future iterations.

So, we will have to wait for this to be fixed, or not.

@dmitshur
Copy link
Member

So the response doesn't really confirm that the type is an integer, does it?

@elliott-beach
Copy link
Contributor

elliott-beach commented Oct 13, 2017

So the response doesn't really confirm that the type is an integer, does it?

I got another email that confirmed that the type is an integer. I also realized that my thinking on this had been backward the whole time - it makes perfect sense for ID to be an int, it should not have required emailing GitHub at all. id == int is the convention the GitHub API has been using.

Copy link
Member

@dmitshur dmitshur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, if you look at say the Get team endpoint, the sample output includes:

"id": 1,

Also based on the error, it's quite clear that it should be "number or null".

GitHub should still make the docs more clear, but I think we have enough to proceed without waiting on further clarification for now.

LGTM.

@dmitshur
Copy link
Member

This has my and Elliott's approvals. I'll merge this PR into NestedTeamsPreview branch and we can continue discussion/review there. Thanks @alindeman!

@dmitshur dmitshur merged commit fcf99d1 into google:NestedTeamsPreview Oct 13, 2017
dmitshur pushed a commit that referenced this pull request Oct 16, 2017
Using the code as written in the `NestedTeamsPreview` branch produces
an error when talking to the GitHub API:

	panic: PATCH https://github.com/api/teams/2514825: 422 Invalid request.

	For 'properties/parent_team_id', "2514824" is not a number or null. []

The documentation at https://developer.github.com/v3/orgs/teams/#create-team
states that this parameter is an id type which appears to mean an integer, not a string.
Also, id is an integer in the sample responses for Get team endpoint, etc.
gmlewis added a commit that referenced this pull request Oct 31, 2017
* Add support for fetching parent team (for nested teams). (#723)

Create mediaTypeNestedTeamsPreview const for
https://developer.github.com/changes/2017-08-30-preview-nested-teams/.

Add parent field to the Team struct.

Set preview API media type in endpoints that return a team, so that
the new parent field gets populated.

Helps #714.

* Add NewTeam struct. (#724)

This replaces Team with NewTeam in function that accept a team for
creating or editing a team. The NewTeam struct is similar to the
existing NewPullRequest for creating pull requests.

We need a new struct because the set of parameters for creating or
editing a team contains some parameters that are not in the parameter
for teams, and most properties of teams cannot be used for editing.

The specific change is that Teams are returned with a parent field,
but are created/edited with a parent_id field.

Helps #714.

* Add NestedTeamsPreview header to remaining endpoints

Fixes #714.

* Change ParentTeamID field to int (from string). (#748)

Using the code as written in the `NestedTeamsPreview` branch produces
an error when talking to the GitHub API:

	panic: PATCH https://github.com/api/teams/2514825: 422 Invalid request.

	For 'properties/parent_team_id', "2514824" is not a number or null. []

The documentation at https://developer.github.com/v3/orgs/teams/#create-team
states that this parameter is an id type which appears to mean an integer, not a string.
Also, id is an integer in the sample responses for Get team endpoint, etc.

* Bump libraryVersion

* NestedTeamsPreview: Add the ListChildTeams endpoint (#758)
nbareil pushed a commit to nbareil/go-github that referenced this pull request May 1, 2018
* Add support for fetching parent team (for nested teams). (google#723)

Create mediaTypeNestedTeamsPreview const for
https://developer.github.com/changes/2017-08-30-preview-nested-teams/.

Add parent field to the Team struct.

Set preview API media type in endpoints that return a team, so that
the new parent field gets populated.

Helps google#714.

* Add NewTeam struct. (google#724)

This replaces Team with NewTeam in function that accept a team for
creating or editing a team. The NewTeam struct is similar to the
existing NewPullRequest for creating pull requests.

We need a new struct because the set of parameters for creating or
editing a team contains some parameters that are not in the parameter
for teams, and most properties of teams cannot be used for editing.

The specific change is that Teams are returned with a parent field,
but are created/edited with a parent_id field.

Helps google#714.

* Add NestedTeamsPreview header to remaining endpoints

Fixes google#714.

* Change ParentTeamID field to int (from string). (google#748)

Using the code as written in the `NestedTeamsPreview` branch produces
an error when talking to the GitHub API:

	panic: PATCH https://github.com/api/teams/2514825: 422 Invalid request.

	For 'properties/parent_team_id', "2514824" is not a number or null. []

The documentation at https://developer.github.com/v3/orgs/teams/#create-team
states that this parameter is an id type which appears to mean an integer, not a string.
Also, id is an integer in the sample responses for Get team endpoint, etc.

* Bump libraryVersion

* NestedTeamsPreview: Add the ListChildTeams endpoint (google#758)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Indication that the PR author has signed a Google Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants