googleapis / gax-ruby

Google API Extensions for Ruby
https://rubygems.org/gems/google-gax
BSD 3-Clause "New" or "Revised" License
20 stars 22 forks source link

Refactor PagedEnumerable #166

Closed blowmage closed 5 years ago

blowmage commented 5 years ago

Update PagedEnumerable to work by convention, not configuration. Paged API calls will be determined by fields existing on the request object and the response object. Remove configuration and simplify the way PagedEnumerable objects are created. We expect the GAPIC client to create instead of GAX.

codecov[bot] commented 5 years ago

Codecov Report

Merging #166 into master will increase coverage by 2.4%. The diff coverage is 95.76%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #166     +/-   ##
=========================================
+ Coverage   95.51%   97.91%   +2.4%     
=========================================
  Files          13       18      +5     
  Lines        1025     1201    +176     
=========================================
+ Hits          979     1176    +197     
+ Misses         46       25     -21
Impacted Files Coverage Δ
lib/google/gax/api_callable.rb 96.96% <ø> (+35.79%) :arrow_up:
test/google/gax/paged_enumerable/enum_test.rb 100% <100%> (ø)
test/google/gax/util_test.rb 100% <100%> (ø) :arrow_up:
...oogle/gax/paged_enumerable/invalid_request_test.rb 100% <100%> (ø)
lib/google/gax.rb 100% <100%> (ø) :arrow_up:
test/fixtures/fixture_pb.rb 100% <100%> (ø) :arrow_up:
...ax/paged_enumerable/valid_request_response_test.rb 100% <100%> (ø)
...ogle/gax/paged_enumerable/invalid_response_test.rb 80.95% <80.95%> (ø)
lib/google/gax/paged_enumerable.rb 96.15% <96.15%> (ø)
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 7cd2a0a...79b9166. Read the comment docs.

blowmage commented 5 years ago

If we go ahead without requiring the response field to be explicitly given do we need to update the tests? Any others?

Funny enough, there are zero tests for PagedEnumerable. So yes, we need to update/add tests. We had discussed migrating from RSpec to Minitest, and I would prefer to add tests using Minitest. I'll add an issue to track the missing tests for PagedEnumerable.