Skip to content
This repository has been archived by the owner on Jun 28, 2022. It is now read-only.

feat: add page streaming for php #3318

Merged
merged 12 commits into from
Dec 24, 2020
Merged

feat: add page streaming for php #3318

merged 12 commits into from
Dec 24, 2020

Conversation

bshaffer
Copy link
Contributor

@bshaffer bshaffer commented Dec 10, 2020

This functionality now works for MapFields!

@bshaffer bshaffer requested a review from a team as a code owner December 10, 2020 18:09
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Dec 10, 2020
@codecov
Copy link

codecov bot commented Dec 10, 2020

Codecov Report

Merging #3318 (aa9c05d) into master (3ff8ca9) will decrease coverage by 0.00%.
The diff coverage is 75.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #3318      +/-   ##
============================================
- Coverage     87.14%   87.13%   -0.01%     
  Complexity     6120     6120              
============================================
  Files           495      495              
  Lines         24172    24173       +1     
  Branches       2637     2638       +1     
============================================
  Hits          21064    21064              
  Misses         2242     2242              
- Partials        866      867       +1     
Impacted Files Coverage Δ Complexity Δ
...google/api/codegen/config/PageStreamingConfig.java 65.59% <66.66%> (-0.72%) 21.00 <0.00> (ø)
...en/transformer/php/PhpGapicSurfaceTransformer.java 93.02% <100.00%> (ø) 46.00 <0.00> (ø)

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 3ff8ca9...03c0bca. Read the comment docs.

Copy link
Contributor

@vchudnov-g vchudnov-g left a comment

Choose a reason for hiding this comment

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

LGTM but have @vam-google TAL

pageSizeField = methodModel.getInputField(pagingParams.getNameForMaxResults());
} else if (language == TargetLanguage.PHP) {
FieldModel resourcesField = ProtoPageStreamingTransformer.getResourcesField(methodModel);
if (resourcesField != null && !resourcesField.isMap()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we not need this in Java because Java is handling map responses?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is my understanding, yes!

Copy link
Contributor

@vam-google vam-google left a comment

Choose a reason for hiding this comment

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

LGTM

@bshaffer bshaffer merged commit 6de594b into master Dec 24, 2020
@bshaffer bshaffer deleted the add-page-streaming-for-php branch December 24, 2020 16:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants