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

fix: array type template #50

Merged

Conversation

shamigor
Copy link
Contributor

@shamigor shamigor commented Jun 2, 2021

Fixes

A short description of what this PR does.

Checklist

  • I acknowledge that all my contributions will be made under the project's license
  • Run make test-docker
  • Verify affected language:
    • Generate twilio-go from our OpenAPI specification using the build_twilio_go.py using python examples/build_twilio_go.py path/to/twilio-oai/spec/yaml path/to/twilio-go and inspect the diff
    • Run make test in twilio-go
    • Create a pull request in twilio-go
    • Provide a link below to the pull request
  • I have made a material change to the repo (functionality, testing, spelling, grammar)
  • I have read the Contribution Guidelines and my PR follows them
  • I have titled the PR appropriately
  • I have updated my branch with the main branch
  • I have added tests that prove my fix is effective or that my feature works
  • I have added the necessary documentation about the functionality in the appropriate .md file
  • I have added inline documentation to the code I modified

If you have questions, please create a GitHub Issue in this repository.

Copy link
Contributor

@shwetha-manvinkurke shwetha-manvinkurke left a comment

Choose a reason for hiding this comment

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

@shamigor Would you be able to generate the twilio-go against the new changes and submit a PR there as well and link it to this one?

@shamigor
Copy link
Contributor Author

shamigor commented Jun 3, 2021

@shamigor Would you be able to generate the twilio-go against the new changes and submit a PR there as well and link it to this one?

Yes, done. PR
Only I'm not sure about the version * API version: 1.16.1. Should I change the version?

Copy link
Contributor

@childish-sambino childish-sambino left a comment

Choose a reason for hiding this comment

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

Looks simple enough. Nice fix.

Think a unit test is needed as well. Since we already have an array-type property in the test fixture, you could add a test which verifies calling the generated code properly sets the payload.

@shamigor
Copy link
Contributor Author

shamigor commented Jun 4, 2021

Looks simple enough. Nice fix.

Think a unit test is needed as well. Since we already have an array-type property in the test fixture, you could add a test which verifies calling the generated code properly sets the payload.

Added. Could you check it please?

Copy link
Contributor

@childish-sambino childish-sambino left a comment

Choose a reason for hiding this comment

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

🏅

@childish-sambino childish-sambino merged commit 3ed5c3a into twilio:main Jun 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants