-
-
Notifications
You must be signed in to change notification settings - Fork 6.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] Add python experimental openapiv3 sample and fix PEP8 formatting issues #4992
[Python] Add python experimental openapiv3 sample and fix PEP8 formatting issues #4992
Conversation
@taxpon @frol @mbohlool @cbornet @kenjones-cisco @tomplus @Jyhess @slash-arun @spacether |
@sebastien-rosset this looks good, thanks for creating it!
|
It tells which shell interpreter to invoke per http://www.linuxandubuntu.com/home/comments-in-bash-learn-bash-part-3 |
But it's not a bash script, it's a makefile. In fact the shebang is ignored because there is an extra space. |
Good point. If you delete it does it still work? |
I will remove it and check. |
It looks like a couple other files are missing, I am adding them:
|
Getting formatting issues:
I will update the python template to fix. |
I also had to add # noqa: E501 in a couple lines in the python api.template because the generated line is too long in openapi3. |
...erator/src/main/java/org/openapitools/codegen/languages/PythonClientExperimentalCodegen.java
Outdated
Show resolved
Hide resolved
modules/openapi-generator/src/main/resources/python/python-experimental/api.mustache
Outdated
Show resolved
Hide resolved
modules/openapi-generator/src/main/resources/python/python-experimental/api.mustache
Outdated
Show resolved
Hide resolved
As a side note, what is the plan to move python-experimental and go-experimental from "experimental" status? Is this going to replace the "python" and "go" generators? Or will there be two separate generators? |
That is a great question. I don't know. My suggestion is to ask on Slack because the maintainers are the most responsive there. If any actions come out of it, then maybe making a feature request to capture it would be a traceable way to track it. My hope is that in the future this can replace the python generator or we can port a bunch of these features into it. We are allowed to make breaking changes on these experimental generators which means we can make progress quickly and aggressively refactor but I don't know what our plan is going forward. |
samples/client/petstore/python-experimental/petstore_api/api/another_fake_api.py
Outdated
Show resolved
Hide resolved
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.
This looks good, the CircleCi failure is an unrelated Maven issue
thanks. I saw the Maven issue you have raised. |
Are all maven issues resolved?
I see the following error in this build, but I have synced from master. There are still many instances of "http://maven" in master. grep -r "http://" . |grep maven |
My PR was an attempt at a maven fix. But I don't know Maven so they may still be unresolved. |
If you close and reopen the pr it will kick off ci again. No need for dummy commits |
thanks. One thing I don't understand is I committed a change this morning, after syncing from master (which has the maven fix). But it still failed with "http" connection to maven. |
Add python-experimental-openapiv3-sample Add missing files for the Python samples Add python-experimental-petstore.bat for openapi v3 Add python-experimental samples openapi v3 Add python-experimental samples openapi v3 Add python-experimental samples openapi v3. Address review comments add missing files for test purpose fix python formatting issues fix python formatting issues fix python formatting issues Fix unit tests fix python formatting issues fix python formatting issues fix python formatting issues fix 'line too long' pep8 error address PR comments for pep8 'line too long' problem regenerate samples execute samples scripts dummy commit to retrigger circleci Revert dummy commit, it didn't help.
0d399dc
to
b381972
Compare
I am looking forward to approving this, just waiting on the CI tests to pass. |
Build is successful now. There were unrelated issues with circleCI, Jim and I fixed the issues. |
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.
This looks good
Thank you! |
PR checklist
./bin/
(or Windows batch scripts under.\bin\windows
) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run./bin/{LANG}-petstore.sh
,./bin/openapi3/{LANG}-petstore.sh
if updating the code or mustache templates for a language ({LANG}
) (e.g. php, ruby, python, etc).master
,4.3.x
,5.0.x
. Default:master
.