-
Notifications
You must be signed in to change notification settings - Fork 6k
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] remove imports from models #7905
base: master
Are you sure you want to change the base?
Conversation
2.3.0-SNAPSHOT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that the version should be 2.4.0
~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mvn clean
was needed 🤦♂️ thanks for spotting this.
@@ -6,10 +6,6 @@ import pprint | |||
import re # noqa: F401 | |||
|
|||
import six | |||
{{#imports}}{{#-first}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about the scenario where the discriminator is used?
The lack of import will result in invalid code due to the missing import.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this discriminator work if modules can't be imported due to circular dependencies ?
Maybe there is another way to provide paths/data for the discriminator - I don't know, I don't use it. Such imports make more problems for standard usage of generated code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kenjones-cisco I dig into it. Finally I've written a test and I also had to fix discriminator to make it working ;)
Now it works without these imports. Child models are created dynamically on the parsing step and models with discriminator don't refer to them - only as a string with name of class.
I still recommend to merge this PR.
Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the status of this PR? I have a similar fix and was about to submit it before stumbling into this PR.
We are seeing models created with circular imports too which makes the generated code useless unless they are manually commented out!
LGTM! Thanks! |
+1 |
PR checklist
./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\
.3.0.0
branch for changes related to OpenAPI spec 3.0. Default:master
.Description of the PR
It resolves the issue #7541 (circular dependencies in Python models), based on @wy-z work, thanks.
PTAL @taxpon (2017/07) @frol (2017/07) @mbohlool (2017/07) @cbornet (2017/09) @kenjones-cisco (2017/11)