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

Update MiscellaneousClient to ApiClient approach and add pagination support to GetAllLicenses. #1716

Conversation

gdziadkiewicz
Copy link
Contributor

@gdziadkiewicz gdziadkiewicz commented Nov 12, 2017

Fixes #1663

  • Remove convention test exclusion
  • Add ApiOptions overload
    • make existing methods call new overload with ApiOptions.None
  • Update unit tests
    • Adjust for ApiOptions parameter
    • Add null/"" checks for ApiOptions overloads
  • Add pagination Integration tests

@gdziadkiewicz gdziadkiewicz force-pushed the feature/#1663_AddPaginationHandlingToMiscellaneousClients branch from 576af11 to 4da7873 Compare November 12, 2017 16:14
@ryangribble
Copy link
Contributor

ryangribble commented Nov 13, 2017

Whoa I didn't even realise we had an ApiClient in the codebase that didn't use the ApiClient base class, nice pickup! 🎉

I guess the only thing is that in order to prove these changes are good, we will need integration tests for all the methods in this client, not just the paginated ones 😉

@shiftkey shiftkey force-pushed the feature/#1663_AddPaginationHandlingToMiscellaneousClients branch from 8e51c89 to 4da21aa Compare February 24, 2020 23:40
@codecov
Copy link

codecov bot commented Feb 24, 2020

Codecov Report

Merging #1716 into master will increase coverage by 0.08%.
The diff coverage is 70.83%.

@@            Coverage Diff             @@
##           master    #1716      +/-   ##
==========================================
+ Coverage   66.86%   66.94%   +0.08%     
==========================================
  Files         542      542              
  Lines       14241    14223      -18     
==========================================
  Hits         9522     9522              
+ Misses       4719     4701      -18
Impacted Files Coverage Δ
Octokit/GitHubClient.cs 80.64% <100%> (ø) ⬆️
....Reactive/Clients/ObservableMiscellaneousClient.cs 78.57% <50%> (+1.64%) ⬆️
Octokit/Helpers/ApiUrls.cs 97.97% <66.66%> (-0.65%) ⬇️
Octokit/Clients/MiscellaneousClient.cs 71.42% <75%> (+11.9%) ⬆️
Octokit/Models/Response/LicenseMetadata.cs 88.23% <0%> (+47.05%) ⬆️

@shiftkey shiftkey force-pushed the feature/#1663_AddPaginationHandlingToMiscellaneousClients branch from 4da21aa to bcd6c1e Compare February 25, 2020 00:09
@shiftkey shiftkey force-pushed the feature/#1663_AddPaginationHandlingToMiscellaneousClients branch from bcd6c1e to fdbe099 Compare February 25, 2020 22:26
@shiftkey shiftkey changed the title [WIP] Update MiscellaneousClient to ApiClient approach and add pagination support to GetAllLicenses. Update MiscellaneousClient to ApiClient approach and add pagination support to GetAllLicenses. Feb 25, 2020
@shiftkey
Copy link
Member

release_notes: add pagination support to client.Miscellaneous.GetAllLicenses();

@shiftkey
Copy link
Member

@gdziadkiewicz thanks for getting this going! I decided to push it over the finish line and add in the missing integration tests. If it all goes green now, I'm gonna merge and include it in the next update.

@shiftkey shiftkey merged commit 04c9a9a into octokit:master Feb 25, 2020
@nickfloyd nickfloyd added Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR and removed category: housekeeping labels Oct 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add missing pagination support to Miscellaneous Clients
4 participants