Skip to content
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

Add "importers" field to MiscellaneousClient.Meta response #1235

Merged
merged 6 commits into from
Apr 4, 2016

Conversation

ryangribble
Copy link
Contributor

Adds new field "Importers" to the Metadata request of MiscellaneousClient

API docs: https://developer.github.com/v3/meta/

Fixes #1234

  • Added Importers field to Meta response model and updated ctor
  • Added unit test for GetMetadata method
  • Enhanced integration tests for GetMetadata method
    • Revealed a bug with an existing field GitHubServicesSha not being deserialised properly, added a fix to this PR
  • Added unit tests for ObservableMiscellaneousClient for all methods
    No tests for this observable client existed, so all methods were implemented
  • Fixed ObservableMiscellaneousClient constructor to be inline with all other obersvable API clients
    (Marked old constructor as [Obsolete] so it can be removed in a later release)

Implement unit test for GetMetadata() call
Flesh out integration test to check all returned Meta fields
-> Found and fixed a bug where the existing `GitHubServicesSHA` field was not deserialised properly
Fix observable's constructor (obsoleting old constructor) to make it consistent with the other API clients
@ryangribble ryangribble changed the title Add importers to meta Add importers field to meta response in MiscellaneousClient Apr 3, 2016
@ryangribble ryangribble changed the title Add importers field to meta response in MiscellaneousClient Add "importers" field to meta response in `MiscellaneousClient Apr 3, 2016
@ryangribble ryangribble changed the title Add "importers" field to meta response in `MiscellaneousClient Add "importers" field to MiscellaneousClient.Meta response Apr 3, 2016
@shiftkey
Copy link
Member

shiftkey commented Apr 3, 2016

Just a heads up: #1238 has a fix for the Travis issue - feel free to cherry-pick 8ec4bf3 so you can verify this PR is still correct

@@ -9,6 +9,7 @@ public class ObservableMiscellaneousClient : IObservableMiscellaneousClient
{
readonly IMiscellaneousClient _client;

[Obsolete("Please use another constructor")]
Copy link
Member

Choose a reason for hiding this comment

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

Let's make this really specific - what about something like: [Obsolete("Please use the IGitHubClient overload constructor")]

@shiftkey
Copy link
Member

shiftkey commented Apr 4, 2016

Revealed a bug with an existing field GitHubServicesSha not being deserialised properly, added a fix to this PR

It's weird that this is failing - the UpperCamelCase to upper_camel_case looks like it should work here but perhaps I'm missing something. Anyway, happy to take this in here...

@shiftkey shiftkey self-assigned this Apr 4, 2016
@shiftkey
Copy link
Member

shiftkey commented Apr 4, 2016

@ryangribble this all seems great ✨ just the little message which is really, really minor

@ryangribble
Copy link
Contributor Author

It's weird that this is failing - the UpperCamelCase to upper_camel_case looks like it should work here but perhaps I'm missing something. Anyway, happy to take this in here...

I think the capital H in "GitHub" was causing it to be looking for git_hub_services_sha rather than github_services_sha

But yeah in any case, it was null before this change, but is now populated correctly. And it's a greeat example of why integration tests should check all fields and not just selected fields 😀

@shiftkey
Copy link
Member

shiftkey commented Apr 4, 2016

I think the capital H in "GitHub" was causing it

Ah yes, that makes sense 💀

@shiftkey
Copy link
Member

shiftkey commented Apr 4, 2016

@shiftkey shiftkey merged commit 4ed32bc into octokit:master Apr 4, 2016
@ryangribble ryangribble deleted the add-importers-to-meta branch April 4, 2016 21:44
@shiftkey shiftkey added the bugfix label Apr 8, 2016
@nickfloyd nickfloyd added Type: Bug Something isn't working as documented and removed category: bug labels Oct 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Something isn't working as documented
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New property on Meta endpoint response
3 participants