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

[python] : Make example code snippet compilable #3148

Merged

Conversation

saigiridhar21
Copy link
Contributor

@saigiridhar21 saigiridhar21 commented Jun 13, 2019

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\. If contributing template-only or documentation-only changes which will change sample output, be sure to build the project first.
  • Filed the PR against the correct branch: master, 4.1.x, 5.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

Making example code snippets compilable.

@taxpon (2017/07) @frol (2017/07) @mbohlool (2017/07) @cbornet (2017/09) @kenjones-cisco (2017/11) @tomplus (2018/10) @Jyhess (2019/01)

@auto-labeler
Copy link

auto-labeler bot commented Jun 13, 2019

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

@@ -6,6 +6,7 @@ from {{{packageName}}}.rest import ApiException
from pprint import pprint
{{> python_doc_auth_partial}}
{{#hasAuthMethods}}
configuration.host = "{{{basePath}}}"
Copy link
Member

Choose a reason for hiding this comment

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

@saigiridhar21 thanks for the PR. Please correct me if I'm wrong. I believe the default of configuration.host is already set to {{{basePath}}}. Why do we need to do it again before passing to ApiClient?:

Copy link
Contributor Author

@saigiridhar21 saigiridhar21 Jun 22, 2019

Choose a reason for hiding this comment

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

@wing328 Yes. You are right. My reason might not be a strong one but it is more like a hint for users that they can update host.
In our use case, we have a horizontally scaled system and we generate SDKs using one of the hosts. So, we don't specify host in the swagger spec. As a result, SDKs get generated with localhost url. A public DNS is there which load balances requests to all the hosts. So, it is expected that clients specify the host in their code.

@wing328
Copy link
Member

wing328 commented Jun 22, 2019 via email

@saigiridhar21
Copy link
Contributor Author

Very good reason. What about also adding a 1 liner comment explaining it's optional and default to {{{basePath}}} ? 在 2019年6月22日週六 上午10:21,Sai Giridhar P notifications@github.com 寫道:

@.**** commented on this pull request. ------------------------------ In modules/openapi-generator/src/main/resources/python/api_doc_example.mustache <#3148 (comment)> : > @@ -6,6 +6,7 @@ from {{{packageName}}}.rest import ApiException from pprint import pprint {{> python_doc_auth_partial}} {{#hasAuthMethods}} +configuration.host = "{{{basePath}}}" @wing328 https://github.com/wing328 Yes. You are right. My reason might not be a strong one but it is more like a hint for users that they can update host. In our use case, we have a horizontally scaled system and we generate SDKs using one of the hosts. We don't specify host in the swagger spec. So, SDKs get generated with localhost url. A public DNS is there which load balances requests to all the hosts. So, it is expected that clients specify the host in their code. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3148?email_source=notifications&email_token=AAHEC5ES4OGHUZJCCGRYMXLP3WECRA5CNFSM4HXXM5LKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB4K2W4Q#discussion_r296430977>, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHEC5DPIAJG57FUAVUFNELP3WECRANCNFSM4HXXM5LA .

Sure I will add a comment

@saigiridhar21
Copy link
Contributor Author

Very good reason. What about also adding a 1 liner comment explaining it's optional and default to {{{basePath}}} ? 在 2019年6月22日週六 上午10:21,Sai Giridhar P notifications@github.com 寫道:

@.**** commented on this pull request. ------------------------------ In modules/openapi-generator/src/main/resources/python/api_doc_example.mustache <#3148 (comment)> : > @@ -6,6 +6,7 @@ from {{{packageName}}}.rest import ApiException from pprint import pprint {{> python_doc_auth_partial}} {{#hasAuthMethods}} +configuration.host = "{{{basePath}}}" @wing328 https://github.com/wing328 Yes. You are right. My reason might not be a strong one but it is more like a hint for users that they can update host. In our use case, we have a horizontally scaled system and we generate SDKs using one of the hosts. We don't specify host in the swagger spec. So, SDKs get generated with localhost url. A public DNS is there which load balances requests to all the hosts. So, it is expected that clients specify the host in their code. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3148?email_source=notifications&email_token=AAHEC5ES4OGHUZJCCGRYMXLP3WECRA5CNFSM4HXXM5LKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB4K2W4Q#discussion_r296430977>, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHEC5DPIAJG57FUAVUFNELP3WECRANCNFSM4HXXM5LA .

Sure I will add a comment

@wing328 Added the comment as suggested. Please take a look.

@wing328
Copy link
Member

wing328 commented Jun 24, 2019

Sorry that I wasn't clear in my reply. Can we add the comment 1 line above the code, which is more consistent with how the rest of the example code is documented?

@saigiridhar21
Copy link
Contributor Author

Sorry that I wasn't clear in my reply. Can we add the comment 1 line above the code, which is more consistent with how the rest of the example code is documented?

@wing328 Made the change as suggested.

@saigiridhar21
Copy link
Contributor Author

@wing328 Please take a look when you get a chance. I made the requested changes. Also, I made the first letter in each comment to capital letter to make it look consistent across all comments on example snippet.

Copy link
Member

@wing328 wing328 left a comment

Choose a reason for hiding this comment

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

LGTM

@wing328
Copy link
Member

wing328 commented Jun 28, 2019

@saigiridhar21 the change looks good. Thanks for the prompt update.

@wing328 wing328 merged commit 6578cef into OpenAPITools:master Jun 28, 2019
@saigiridhar21 saigiridhar21 deleted the fix/python/make-example-compilable branch August 20, 2019 02:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants