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][client] Comment out useless imports #7542

Closed
wants to merge 2 commits into from
Closed

[python][client] Comment out useless imports #7542

wants to merge 2 commits into from

Conversation

wy-z
Copy link
Contributor

@wy-z wy-z commented Jan 31, 2018

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 and ./bin/security/{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\.
  • Filed the PR against the correct branch: 3.0.0 branch for changes related to OpenAPI spec 3.0. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

wy-z added 2 commits January 31, 2018 16:37
Signed-off-by: weiyang <weiyang.ones@gmail.com>
Signed-off-by: weiyang <weiyang.ones@gmail.com>
@wy-z wy-z changed the title Fix python client [python][client] Comment out useless imports to avoid circular importation Jan 31, 2018
@wy-z wy-z changed the title [python][client] Comment out useless imports to avoid circular importation [python][client] Comment out imports Jan 31, 2018
@wy-z wy-z changed the title [python][client] Comment out imports [python][client] Comment out useless imports Jan 31, 2018
@cbornet
Copy link
Contributor

cbornet commented Jan 31, 2018

These imports are at least used for the docstrings so I don’t think they can be removed like this.

@wy-z
Copy link
Contributor Author

wy-z commented Feb 1, 2018

See #7541 , any PR is welcome~

@wing328
Copy link
Contributor

wing328 commented Feb 1, 2018

@wy-z I think a proper way to do it is to loop through the imports in postProcessModels and remove it if it's the same as the model name.

@wing328
Copy link
Contributor

wing328 commented Feb 1, 2018

Sorry we've to close this one. Please open a new one with the suggested fix.

@tomplus
Copy link
Contributor

tomplus commented Mar 22, 2018

@cbornet AFAIK docstrings don't require imports. Python complains about these imports too (it's the reason why there is F401 directive for pylint).

@wing328 removing self-import was implemented by removing the first item from the list. We can do it better as you suggest, but the biggest problem is with circular dependencies. I have no idea how to fix it - it'll simpler to delete it.

@wing328
Copy link
Contributor

wing328 commented Mar 25, 2018

@tomplus right. Let's go with removing it first.

@wy-z If there's a minimal spec to reproduce the issue, please share as it can help verify the fix later.

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.

4 participants