Skip to content

avoid setting debug property if not needed #18872

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.

Sign up for GitHub

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
merged 8 commits into from
Jun 20, 2024
Merged

avoid setting debug property if not needed #18872

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b34c7e0
avoid setting debug property if not needed
ZeeD Jun 6, 2024
ad984d2
update samples
ZeeD Jun 6, 2024
c12633f
fix compatibility with python 3.7
ZeeD Jun 12, 2024
da70b93
always set Configuration.__debug
ZeeD Jun 17, 2024
2f719a3
update samples
ZeeD Jun 17, 2024
b269b63
check `Configuration` behavior when debug parameter is / is not set
ZeeD Jun 17, 2024
197a492
address PR requests
ZeeD Jun 17, 2024
1687849
Merge remote-tracking branch 'origin/master' into patch-1
ZeeD Jun 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion modules/openapi-generator/src/main/resources/python/configuration.mustache
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,8 @@ conf = {{{packageName}}}.Configuration(
server_operation_index=None, server_operation_variables=None,
ssl_ca_cert=None,
retries=None,
*,
debug: Optional[bool] = None
) -> None:
"""Constructor
"""
Expand Down Expand Up @@ -214,7 +216,10 @@ conf = {{{packageName}}}.Configuration(
self.logger_file = None
"""Debug file location
"""
self.debug = False
if debug is not None:
self.debug = debug
Comment on lines +219 to +220
Copy link
Contributor

Choose a reason for hiding this comment

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

If this self.debug property is not always set, then the underlying __debug property will also not always be set, which may produce AttributeError.

I would suggest to at least set self.__debug to False by default so that the debug setter/getter don't have a way to fail, and all the code that are also using this attribute will also not fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can easily add a default value for self.__debug, but I'm not sure how to write the tests.
Can you point me to a sample I can adapt?

Copy link
Contributor

Choose a reason for hiding this comment

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

You can check here for instance: https://github.com/OpenAPITools/openapi-generator/blob/master/samples/openapi3/client/petstore/python/tests/test_configuration.py

There are several tests around this class, you should be able to create a test that fails with your current change and make it pass by setting an initial value for __debug.

There may be other places with this kind of tests (they are written manually), but I didn't check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uhm... I was starting to write some tests, but it seems there are some unrelated failures.

$ cd samples/openapi3/client/petstore/python
$ poetry install
[...]
$ poetry run pytest
[...]
================================================= short test summary info ==================================================
FAILED tests/test_model.py::ModelTests::test_valdiator - AttributeError: 'ModelTests' object has no attribute 'assertEquals'. Did you mean: 'assertEqual'?
FAILED tests/test_pet_api.py::PetApiTests::test_add_pet_and_get_pet_by_id - urllib3.exceptions.MaxRetryError: HTTPConnectionPool(host='localhost', port=80): Max retries exceeded with url: /v2/pet...
FAILED tests/test_pet_api.py::PetApiTests::test_add_pet_and_get_pet_by_id_with_http_info - urllib3.exceptions.MaxRetryError: HTTPConnectionPool(host='localhost', port=80): Max retries exceeded with url: /v2/pet...
FAILED tests/test_pet_api.py::PetApiTests::test_add_pet_and_get_pet_by_id_without_preload_content_flag - urllib3.exceptions.MaxRetryError: HTTPConnectionPool(host='localhost', port=80): Max retries exceeded with url: /v2/pet...
FAILED tests/test_pet_api.py::PetApiTests::test_delete_pet - urllib3.exceptions.MaxRetryError: HTTPConnectionPool(host='localhost', port=80): Max retries exceeded with url: /v2/pet...
FAILED tests/test_pet_api.py::PetApiTests::test_find_pets_by_status - urllib3.exceptions.MaxRetryError: HTTPConnectionPool(host='localhost', port=80): Max retries exceeded with url: /v2/pet...
FAILED tests/test_pet_api.py::PetApiTests::test_find_pets_by_tags - urllib3.exceptions.MaxRetryError: HTTPConnectionPool(host='localhost', port=80): Max retries exceeded with url: /v2/pet...
FAILED tests/test_pet_api.py::PetApiTests::test_get_pet_by_id_serialize - urllib3.exceptions.MaxRetryError: HTTPConnectionPool(host='localhost', port=80): Max retries exceeded with url: /v2/pet...
FAILED tests/test_pet_api.py::PetApiTests::test_update_pet - urllib3.exceptions.MaxRetryError: HTTPConnectionPool(host='localhost', port=80): Max retries exceeded with url: /v2/pet...
FAILED tests/test_pet_api.py::PetApiTests::test_update_pet_with_form - urllib3.exceptions.MaxRetryError: HTTPConnectionPool(host='localhost', port=80): Max retries exceeded with url: /v2/pet...
FAILED tests/test_pet_api.py::PetApiTests::test_upload_file - urllib3.exceptions.MaxRetryError: HTTPConnectionPool(host='localhost', port=80): Max retries exceeded with url: /v2/pet...
================================== 11 failed, 264 passed, 3 skipped, 9 warnings in 14.25s ==================================

I'm normally using python 3.12, where it was removed
Should I also update the unrelated tests?

Copy link
Contributor

Choose a reason for hiding this comment

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

  • For assertEquals, this should be migrated in another PR I think. Can you test with 3.11 instead?
  • For the URL failures, there's an explanation how to start a test container to support the tests at the top of tests/test_pet_api.py

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(oh, and for the various PetApiTests failed tests, while it's easy to follow the instructions in the tests

openapi-generator/samples/openapi3/client/petstore/python/tests/test_pet_api.py

Lines 6 to 11 in 793aba7

Run the tests.
$ docker pull swaggerapi/petstore
$ docker run -d -e SWAGGER_HOST=http://petstore.swagger.io -e SWAGGER_BASE_PATH=/v2 -p 80:8080 swaggerapi/petstore
$ pip install -U pytest
$ cd petstore_api-python
$ pytest
I would suggest to enhance them with testcontainers so you can avoid launching the docker image by hand

else:
self.__debug = False
"""Debug switch
"""

Expand Down
7 changes: 6 additions & 1 deletion .../echo_api/python-disallowAdditionalPropertiesIfNotPresent/openapi_client/configuration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,8 @@ def __init__(self, host=None,
server_operation_index=None, server_operation_variables=None,
ssl_ca_cert=None,
retries=None,
*,
debug: Optional[bool] = None
) -> None:
"""Constructor
"""
Expand Down Expand Up @@ -143,7 +145,10 @@ def __init__(self, host=None,
self.logger_file = None
"""Debug file location
"""
self.debug = False
if debug is not None:
self.debug = debug
else:
self.__debug = False
"""Debug switch
"""

Expand Down
7 changes: 6 additions & 1 deletion samples/client/echo_api/python/openapi_client/configuration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,8 @@ def __init__(self, host=None,
server_operation_index=None, server_operation_variables=None,
ssl_ca_cert=None,
retries=None,
*,
debug: Optional[bool] = None
) -> None:
"""Constructor
"""
Expand Down Expand Up @@ -143,7 +145,10 @@ def __init__(self, host=None,
self.logger_file = None
"""Debug file location
"""
self.debug = False
if debug is not None:
self.debug = debug
else:
self.__debug = False
"""Debug switch
"""

Expand Down
7 changes: 6 additions & 1 deletion samples/openapi3/client/petstore/python-aiohttp/petstore_api/configuration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,8 @@ def __init__(self, host=None,
server_operation_index=None, server_operation_variables=None,
ssl_ca_cert=None,
retries=None,
*,
debug: Optional[bool] = None
) -> None:
"""Constructor
"""
Expand Down Expand Up @@ -207,7 +209,10 @@ def __init__(self, host=None,
self.logger_file = None
"""Debug file location
"""
self.debug = False
if debug is not None:
self.debug = debug
else:
self.__debug = False
"""Debug switch
"""

Expand Down
7 changes: 6 additions & 1 deletion samples/openapi3/client/petstore/python/petstore_api/configuration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,8 @@ def __init__(self, host=None,
server_operation_index=None, server_operation_variables=None,
ssl_ca_cert=None,
retries=None,
*,
debug: Optional[bool] = None
) -> None:
"""Constructor
"""
Expand Down Expand Up @@ -208,7 +210,10 @@ def __init__(self, host=None,
self.logger_file = None
"""Debug file location
"""
self.debug = False
if debug is not None:
self.debug = debug
else:
self.__debug = False
"""Debug switch
"""

Expand Down
9 changes: 9 additions & 0 deletions samples/openapi3/client/petstore/python/tests/test_configuration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,5 +76,14 @@ def test_get_host_from_settings(self):
self.assertEqual("http://petstore.swagger.io:8080/v2", self.config.get_host_from_settings(0, {'port': '8080'}))
self.assertEqual("http://dev-petstore.swagger.io:8080/v2", self.config.get_host_from_settings(0, {'server': 'dev-petstore', 'port': '8080'}))

def testConfigurationDebug(self):
for debug, expected in [(True, True), (False, False), (None, False)]:
with self.subTest('expicitly passing debug parameter', debug=debug, expected=expected):
c = petstore_api.Configuration(debug=debug)
self.assertEqual(expected, c.debug)
with self.subTest('not passing debug parameter'):
c = petstore_api.Configuration()
self.assertFalse(c.debug)

if __name__ == '__main__':
unittest.main()
Loading