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

kenjones-cisco · 2017-10-28T03:00:25Z

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

kenjones-cisco · 2017-10-28T03:02:59Z

modules/swagger-codegen/src/main/java/io/swagger/codegen/languages/PythonClientCodegen.java

@@ -189,21 +190,16 @@ public void processOpts() {
            setPackageUrl((String) additionalProperties.get(PACKAGE_URL));
        }

-        String swaggerFolder = packageName;
-
-        modelPackage = swaggerFolder + File.separatorChar + "models";


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.

kenjones-cisco · 2017-10-28T03:03:57Z

modules/swagger-codegen/src/main/java/io/swagger/codegen/languages/PythonClientCodegen.java

    }

    private static String dropDots(String str) {
        return str.replaceAll("\\.", "_");
    }

    @Override
+    public String toModelImport(String name) {


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

kenjones-cisco · 2017-10-28T03:04:58Z

modules/swagger-codegen/src/main/resources/python/__init__package.mustache

 {{>partial_header}}

 from __future__ import absolute_import

-# import models into sdk package
-{{#models}}{{#model}}from .models.{{classFilename}} import {{classname}}


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

kenjones-cisco · 2017-10-28T03:07:09Z

modules/swagger-codegen/src/main/resources/python/api_client.mustache


        self.last_response = response_data
+{{^tornado}}


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

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

kenjones-cisco · 2017-10-28T03:08:23Z

modules/swagger-codegen/src/main/resources/python/asyncio/rest.mustache

@@ -53,13 +48,15 @@ class RESTClientObject:
            ca_certs = certifi.where()

        ssl_context = ssl.SSLContext()
+        ssl_context.load_verify_locations(cafile=ca_certs)


Resolves the missing use of SSL when using asyncio.

kenjones-cisco · 2017-10-28T03:09:47Z

modules/swagger-codegen/src/main/resources/python/model.mustache

    }
 {{#discriminator}}

    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}},


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

kenjones-cisco · 2017-10-28T03:11:51Z

modules/swagger-codegen/src/main/resources/python/tornado/rest.mustache

@@ -55,9 +50,10 @@ class RESTClientObject:
            # if not set certificate file, use Mozilla's root certificates.
            ca_certs = certifi.where()

-        self.ssl_context = ssl_context = ssl.SSLContext()
+        self.ssl_context = ssl.SSLContext()
+        self.ssl_context.load_verify_locations(cafile=ca_certs)


Adds the missing use of ca_certs.

frol · 2017-10-28T10:33:30Z

modules/swagger-codegen/src/main/resources/python/api_test.mustache


    def setUp(self):
-        self.api = {{packageName}}.apis.{{classVarName}}.{{classname}}()
+        self.api = {{packageName}}.apis.{{classVarName}}.{{classname}}()  # noqa: E501


Should it use apiPackage here?

great catch

frol · 2017-10-28T12:56:33Z

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.

frol · 2017-10-28T13:02:39Z

modules/swagger-codegen/src/main/resources/python/model.mustache

-        {{#vars}}'{{name}}': '{{{datatype}}}'{{#hasMore}},
-        {{/hasMore}}{{/vars}}
+{{#vars}}
+        '{{name}}': {{{datatype}}}{{#hasMore}},{{/hasMore}}


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

kenjones-cisco · 2017-10-28T13:28:40Z

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 · 2017-10-28T14:02:55Z

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.

frol · 2017-10-28T14:37:36Z

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

frol · 2017-10-28T14:45:46Z

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.

wing328 · 2017-10-30T15:46:15Z

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.

kenjones-cisco · 2017-10-31T03:27:35Z

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

kenjones-cisco · 2017-10-31T04:00:51Z

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.

frol · 2017-10-31T06:07:59Z

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

kenjones-cisco · 2017-10-31T12:21:26Z

samples/client/petstore/python/test_python2.sh

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

+### static analysis of code
+flake8 --show-source petstore_api/


Static analysis performed on client as part of CI testing

kenjones-cisco · 2017-10-31T12:21:34Z

samples/client/petstore/python/test_python2_and_3.sh

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

+### static analysis of code


Static analysis performed on client as part of CI testing

kenjones-cisco · 2017-10-31T12:23:11Z

@frol I did update the CI scripts

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

frol · 2017-10-31T12:43:06Z

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

@wing328 This PR is awesome and it is ready to merge.

tomplus · 2017-11-02T17:25:51Z

samples/client/petstore/python-asyncio/petstore_api/__init__.py


 # import ApiClient
-from .api_client import ApiClient
-
-from .configuration import Configuration


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

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.

sbko · 2017-12-04T20:13:36Z

modules/swagger-codegen/src/main/resources/python/__init__model.mustache

 {{>partial_header}}

 from __future__ import absolute_import

 # import models into model package
-{{#models}}{{#model}}from .{{classFilename}} import {{classname}}{{/model}}
+{{#models}}{{#model}}from {{modelPackage}}.{{classFilename}} import {{classname}}{{/model}}


@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

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.

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 :)

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

kenjones-cisco commented Oct 28, 2017

View reviewed changes

kenjones-cisco force-pushed the task/pyclient-lint-errors branch from 11ebea2 to b0668f6 Compare October 28, 2017 03:27

kenjones-cisco mentioned this pull request Oct 28, 2017

[Python] Align with PEP8, PEP257 #5995

Open

frol reviewed Oct 28, 2017

View reviewed changes

frol suggested changes Oct 28, 2017

View reviewed changes

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

kenjones-cisco force-pushed the task/pyclient-lint-errors branch from 906e0e9 to eea711c Compare October 31, 2017 03:26

kenjones-cisco commented Oct 31, 2017

View reviewed changes

wing328 added Client: Python Enhancement: Code format labels Nov 1, 2017

wing328 added this to the v2.3.0 milestone Nov 1, 2017

wing328 merged commit 74f70a1 into swagger-api:master Nov 1, 2017

tomplus reviewed Nov 2, 2017

View reviewed changes

wing328 added the Breaking change (with fallback) label Nov 3, 2017

kenjones-cisco deleted the task/pyclient-lint-errors branch November 6, 2017 01:50

sbko reviewed Dec 4, 2017

View reviewed changes

jimmyjames mentioned this pull request Feb 5, 2018

[bug][python][model] Should not import self #7541

Open

twosigmajab mentioned this pull request Sep 25, 2018

Use relative imports in generated Python packages #8738

Open

roycaihw mentioned this pull request Aug 14, 2019

[DO NOT MERGE] Switch to openapi-generator kubernetes-client/python#926

Closed

roycaihw mentioned this pull request Oct 1, 2019

[RFC] Major breaking changes in the upcoming release kubernetes-client/python#974

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

kenjones-cisco commented Oct 28, 2017 •

edited

Loading

kenjones-cisco Oct 28, 2017

kenjones-cisco Oct 28, 2017

kenjones-cisco Oct 28, 2017

kenjones-cisco Oct 28, 2017

tomplus Nov 5, 2017

kenjones-cisco Oct 28, 2017

kenjones-cisco Oct 28, 2017

kenjones-cisco Oct 28, 2017

frol Oct 28, 2017

kenjones-cisco Oct 28, 2017

frol commented Oct 28, 2017

frol Oct 28, 2017

kenjones-cisco commented Oct 28, 2017 •

edited

Loading

kenjones-cisco commented Oct 28, 2017

frol commented Oct 28, 2017

frol commented Oct 28, 2017

wing328 commented Oct 30, 2017

kenjones-cisco commented Oct 31, 2017

kenjones-cisco commented Oct 31, 2017

frol commented Oct 31, 2017

kenjones-cisco Oct 31, 2017

kenjones-cisco Oct 31, 2017

kenjones-cisco commented Oct 31, 2017

frol commented Oct 31, 2017

tomplus Nov 2, 2017

kenjones-cisco Nov 2, 2017

sbko Dec 4, 2017

kenjones-cisco Dec 4, 2017

sbko Dec 4, 2017

twosigmajab Sep 25, 2018

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

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

Conversation

kenjones-cisco commented Oct 28, 2017 • edited Loading

PR checklist

Description of the PR

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frol commented Oct 28, 2017

Choose a reason for hiding this comment

kenjones-cisco commented Oct 28, 2017 • edited Loading

kenjones-cisco commented Oct 28, 2017

frol commented Oct 28, 2017

frol commented Oct 28, 2017

wing328 commented Oct 30, 2017

kenjones-cisco commented Oct 31, 2017

kenjones-cisco commented Oct 31, 2017

frol commented Oct 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kenjones-cisco commented Oct 31, 2017

frol commented Oct 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kenjones-cisco commented Oct 28, 2017 •

edited

Loading

kenjones-cisco commented Oct 28, 2017 •

edited

Loading