Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

[python-client] Modify python templates to resolve linting errors #6839

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

Merged

Conversation

@kenjones-cisco
Copy link
Contributor

@kenjones-cisco kenjones-cisco commented Oct 28, 2017 •
edited
Loading

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 langauge.

Description of the PR

(details of the change, additional tests that have been done, reference to the issue for tracking, etc)

The linting results for the generated samples are as follows
where the first number is the BEFORE and the second is AFTER.

pyclient 7714 vs. 120
pyclient3 7717 vs. 120
pyclient3-asyncio 7584 vs. 120
pyclient-tornado 7633 vs. 120
pyclient3-tornado 7633 vs. 120

For the complete details please see the following gist.
https://gist.github.com/kenjones-cisco/2eb69a7e8db75e9fd53789f01570d9f2

@taxpon @frol @mbohlool @cbornet
CC @wing328

wing328 reacted with thumbs up emoji

String swaggerFolder = packageName;

modelPackage = swaggerFolder + File.separatorChar + "models";
Copy link
Contributor Author

@kenjones-cisco kenjones-cisco Oct 28, 2017

Choose a reason for hiding this comment

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

These were overriding the real values of modelPackage and apiPackage configurations as specified by user.
Also resulted in not being able to leverage for creating import statements.

}

@Override
public String toModelImport(String name) {
Copy link
Contributor Author

@kenjones-cisco kenjones-cisco Oct 28, 2017

Choose a reason for hiding this comment

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

Added from server implementation to enable the use of model imports like done within server.

from __future__ import absolute_import

# import models into sdk package
{{#models}}{{#model}}from .models.{{classFilename}} import {{classname}}
Copy link
Contributor Author

@kenjones-cisco kenjones-cisco Oct 28, 2017

Choose a reason for hiding this comment

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

hardcoded models and apis were overriding the values specified by user during configuration.

_request_timeout=_request_timeout)

self.last_response = response_data
{{^tornado}}
Copy link
Contributor Author

@kenjones-cisco kenjones-cisco Oct 28, 2017

Choose a reason for hiding this comment

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

Resulted in a return statement within a generator which happens when tornado used.

Copy link
Contributor

@tomplus tomplus Nov 5, 2017

Choose a reason for hiding this comment

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

I don't understand it. Who calls deserialize ? This method returns None now.

ca_certs = certifi.where()

ssl_context = ssl.SSLContext()
ssl_context.load_verify_locations(cafile=ca_certs)
Copy link
Contributor Author

@kenjones-cisco kenjones-cisco Oct 28, 2017

Choose a reason for hiding this comment

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

Resolves the missing use of SSL when using asyncio.


discriminator_value_class_map = {
{{#children}}'{{vendorExtensions.x-discriminator-value}}': '{{{classname}}}'{{^-last}},
{{#children}}'{{^vendorExtensions.x-discriminator-value}}{{name}}{{/vendorExtensions.x-discriminator-value}}{{#vendorExtensions.x-discriminator-value}}{{{vendorExtensions.x-discriminator-value}}}{{/vendorExtensions.x-discriminator-value}}': '{{{classname}}}'{{^-last}},
Copy link
Contributor Author

@kenjones-cisco kenjones-cisco Oct 28, 2017

Choose a reason for hiding this comment

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

Resolves issue where if the user doesn't specify x-discriminator-value and the key value was always an empty string.


self.ssl_context = ssl_context = ssl.SSLContext()
self.ssl_context = ssl.SSLContext()
self.ssl_context.load_verify_locations(cafile=ca_certs)
Copy link
Contributor Author

@kenjones-cisco kenjones-cisco Oct 28, 2017

Choose a reason for hiding this comment

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

Adds the missing use of ca_certs.


def setUp(self):
self.api = {{packageName}}.apis.{{classVarName}}.{{classname}}()
self.api = {{packageName}}.apis.{{classVarName}}.{{classname}}() # noqa: E501
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it use apiPackage here?

Copy link
Contributor Author

@kenjones-cisco kenjones-cisco Oct 28, 2017

Choose a reason for hiding this comment

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

great catch

Copy link
Contributor

frol commented Oct 28, 2017

Overall, the PR looks good to me.

@kenjones-cisco What are the left linter warnings that are not addressed yet? If we squash all the linter messages, we can make linting required to pass CI.

{{#vars}}'{{name}}': '{{{datatype}}}'{{#hasMore}},
{{/hasMore}}{{/vars}}
{{#vars}}
'{{name}}': {{{datatype}}}{{#hasMore}},{{/hasMore}}
Copy link
Contributor

Choose a reason for hiding this comment

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

The {{{datatype}}} has to be quoted as it should be a string, otherwise, it generates a broken code.

Copy link
Contributor Author

kenjones-cisco commented Oct 28, 2017 •
edited
Loading

The remaining ones are because the imports are there but the implementation does not currently exist to use the imported modules. (ie within the api tests generated)

@kenjones-cisco kenjones-cisco force-pushed the task/pyclient-lint-errors branch 2 times, most recently from 042f49e to 906e0e9 Compare October 28, 2017 14:02
Copy link
Contributor Author

After the updates recommended and an ignore to handle to difference between python2 vs python3, each client has the same number of lint errors, 120.

120 are unused imports in the generated test modules.

Copy link
Contributor

frol commented Oct 28, 2017

@wing328 Do you happen to know how to fix Travis?

adding /Users/travis/.cocoapods/repos/master to cache
Password:
No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.

Copy link
Contributor

frol commented Oct 28, 2017

Since the generated tests are just placeholders, I would like to suggest we just drop them from linting altogether (exclude the test folder completely), and enforce linting on CI.
kenjones-cisco reacted with thumbs up emoji

Copy link
Contributor

wing328 commented Oct 30, 2017

No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.

@frol I've seen this many times recently. Let me try something out tomorrow to see if that helps.

 

The linting results for the generated samples are as follows
where the first number is the BEFORE and the second is AFTER.
pyclient 7714 vs. 120
pyclient3 7717 vs. 120
pyclient3-asyncio 7584 vs. 120
pyclient-tornado 7633 vs. 120
pyclient3-tornado 7633 vs. 120
For the complete details please see the following gist.
https://gist.github.com/kenjones-cisco/2eb69a7e8db75e9fd53789f01570d9f2
Enforces linting for python clients by running flake8 for the generated
python client.
Copy link
Contributor Author

@frol Update the test scripts for python client to run flake8 over the generated python client. petstore_api thereby excluding any files within test/

Copy link
Contributor Author

Looks like all jobs were successful, not sure why Shippable is not updating the status on the PR but the Job says Success when looking at it.

Copy link
Contributor

frol commented Oct 31, 2017

@kenjones-cisco Could you, please, update CI scripts so Python samples get linted automatically and fail if any warning is there? This way we will be sure that we won't shift from the guidelines in future.

@@ -23,6 +23,9 @@ python setup.py develop
### run tests
nosetests

### static analysis of code
flake8 --show-source petstore_api/
Copy link
Contributor Author

@kenjones-cisco kenjones-cisco Oct 31, 2017

Choose a reason for hiding this comment

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

Static analysis performed on client as part of CI testing

@@ -23,6 +23,9 @@ python setup.py develop
### run tests
tox

### static analysis of code
Copy link
Contributor Author

@kenjones-cisco kenjones-cisco Oct 31, 2017

Choose a reason for hiding this comment

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

Static analysis performed on client as part of CI testing

Copy link
Contributor Author

@frol I did update the CI scripts

I just added a review statement for the lines in this PR for the CI scripts.

Copy link
Contributor

frol commented Oct 31, 2017

@kenjones-cisco Great! Thank you for your work!

@wing328 This PR is awesome and it is ready to merge.
wing328 reacted with thumbs up emoji

@wing328 wing328 added this to the v2.3.0 milestone Nov 1, 2017
@wing328 wing328 merged commit 74f70a1 into swagger-api:master Nov 1, 2017
# import ApiClient
from .api_client import ApiClient

from .configuration import Configuration
Copy link
Contributor

@tomplus tomplus Nov 2, 2017

Choose a reason for hiding this comment

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

It looks like breaking changes. Some libraries embed generated code and use relative imports.
Change apis to api can be a problem too.

sbko reacted with thumbs up emoji
Copy link
Contributor Author

@kenjones-cisco kenjones-cisco Nov 2, 2017

Choose a reason for hiding this comment

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

The apis to api was actually a bugfix as the default was defined as api but a typo later in the Java code for generating code had a hard coded path of apis.


# import models into model package
{{#models}}{{#model}}from .{{classFilename}} import {{classname}}{{/model}}
{{#models}}{{#model}}from {{modelPackage}}.{{classFilename}} import {{classname}}{{/model}}
Copy link
Contributor

Choose a reason for hiding this comment

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

@wing328 this change broke our use case - we generate code and put it inside our library, so we don't really install swagger generated code itself. so now we get import errors because "swagger_client" is missing

Copy link
Contributor Author

@kenjones-cisco kenjones-cisco Dec 4, 2017

Choose a reason for hiding this comment

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

swagger_client is just the default top level package name, as part of the inputs when generating the code you can specify the package name to match the package name you leverage within your code.

Copy link
Contributor

Choose a reason for hiding this comment

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

current structure:
some_client/swagger_client
some_client/init.py
some_client/client.py - where we import swagger_client stuff
....

change to the name will not fix the problem unfortunately, we need those relative imports :)

Copy link

@twosigmajab twosigmajab Sep 25, 2018

Choose a reason for hiding this comment

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

I just opened #8738 to address the need for relative imports

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

4 more reviewers

@tomplus tomplus tomplus left review comments

@sbko sbko sbko left review comments

@twosigmajab twosigmajab twosigmajab left review comments

@frol frol frol requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

v2.3.0

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /