feat: twilio-python token pagination support #729
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds token pagination support for the twilio-python SDK by introducing conditional logic to use TokenPagination base class for V1 API standard resources instead of the standard Page class. The changes centralize V1 API detection logic in TwilioCodegenAdapter and apply it across Python and PHP generators.
Key changes:
- Refactored V1 API standard detection into a shared
setIsV1ApiStandardmethod inTwilioCodegenAdapter - Updated Python templates to conditionally inherit from
TokenPaginationand passuriparameter for V1 APIs - Modified Python templates to exclude
page_tokenfrom method parameters when explicitly defined
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/resources/twilio-python/pagination.handlebars | Conditionally inherits from TokenPagination for V1 APIs |
| src/main/resources/twilio-python/listOperations.handlebars | Filters page_token parameter and adds uri parameter to V1 API page constructors |
| src/main/resources/twilio-python/api-single.mustache | Conditionally imports TokenPagination instead of Page for V1 APIs |
| src/main/java/com/twilio/oai/TwilioPythonGenerator.java | Calls setIsV1ApiStandard to detect V1 API standard |
| src/main/java/com/twilio/oai/TwilioPhpGenerator.java | Refactored to use shared setIsV1ApiStandard method |
| src/main/java/com/twilio/oai/TwilioCodegenAdapter.java | Added setIsV1ApiStandard method and explicit constructor, removed Lombok annotation |
| examples/node/src/rest/api/v2010/account/call/feedbackCallSummary.ts | Formatting change to class declaration (unrelated to main feature) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return {{apiName}}Page(self._version, response{{#if listPathParams}}, self._solution{{/if}}){{/isApiV1}} | ||
|
|
||
| async def page_async(self, {{#allParams}} | ||
| {{#startsWith paramName text='page_size'}}{{else}}{{paramName}}: Union[{{{dataType}}}, object] = values.unset,{{/startsWith}}{{/allParams}} |
There was a problem hiding this comment.
The page_async method is missing the same page_token parameter filtering that was added to the page method at line 334. For consistency, this line should also exclude page_token from the parameter list:
{{#startsWith paramName text='page_size'}}{{else}}{{#startsWith paramName text='page_token'}}{{else}}{{paramName}}: Union[{{{dataType}}}, object] = values.unset,{{/startsWith}}{{/startsWith}}{{/allParams}}| {{#startsWith paramName text='page_size'}}{{else}}{{paramName}}: Union[{{{dataType}}}, object] = values.unset,{{/startsWith}}{{/allParams}} | |
| {{#startsWith paramName text='page_size'}}{{else}}{{#startsWith paramName text='page_token'}}{{else}}{{paramName}}: Union[{{{dataType}}}, object] = values.unset,{{/startsWith}}{{/startsWith}}{{/allParams}} |
| return version; | ||
| } | ||
|
|
||
| public void setIsV1ApiStandard (final OpenAPI openAPI) { |
There was a problem hiding this comment.
There's an extra space before the opening parenthesis in the method name. It should be setIsV1ApiStandard(final OpenAPI openAPI) instead of setIsV1ApiStandard (final OpenAPI openAPI).
| public void setIsV1ApiStandard (final OpenAPI openAPI) { | |
| public void setIsV1ApiStandard(final OpenAPI openAPI) { |
tiwarishubham635
left a comment
tiwarishubham635
left a comment
There was a problem hiding this comment.
LGTM! Added a few comments for discussion
| twilioCodegen.setDomain(domain); | ||
| twilioCodegen.setVersion(version); | ||
| twilioCodegen.setOutputDir(domain, version); | ||
| twilioCodegen.setIsV1ApiStandard(openAPI); |
There was a problem hiding this comment.
This design looks much better to keep the resource cache code in adapter. Have you checked if this is present in all languages except java?
There was a problem hiding this comment.
yes, we can support this in all generators
| response = self._version.page(method='{{vendorExtensions.x-http-method}}', uri=self._uri, params=data, headers=headers) | ||
| return {{apiName}}Page(self._version, response{{#if listPathParams}}, self._solution{{/if}}) | ||
| response = self._version.page(method='{{vendorExtensions.x-http-method}}', uri=self._uri, params=data, headers=headers){{#isApiV1}} | ||
| return {{apiName}}Page(self._version, response, uri=self._uri{{#if listPathParams}}, self._solution{{/if}}){{/isApiV1}}{{^isApiV1}} |
There was a problem hiding this comment.
i think the only difference is that we are passing the url as well when it's token pagination right?
Fixes
PR for python twilio/twilio-python#896
As part of this PR
Added method setIsV1ApiStandard to TwilioCodegenAdapter, so that it is accessible across different generators
Added inheriting TokenPagination class when IsV1Api is true
Checklist
make test-dockerpython scripts/build_twilio_library.py path/to/twilio-oai/spec/yaml path/to/twilio-java -l javaand inspect the diffmake testintwilio-javatwilio-javaIf you have questions, please create a GitHub Issue in this repository.