-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
avoid setting debug property if not needed #18872
Changes from 6 commits
b34c7e0
ad984d2
c12633f
da70b93
2f719a3
b269b63
197a492
1687849
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -137,6 +137,7 @@ conf = {{{packageName}}}.Configuration( | |||||||||||||
""" | ||||||||||||||
|
||||||||||||||
_default = None | ||||||||||||||
__debug = False | ||||||||||||||
|
||||||||||||||
def __init__(self, host=None, | ||||||||||||||
api_key=None, api_key_prefix=None, | ||||||||||||||
|
@@ -148,6 +149,8 @@ conf = {{{packageName}}}.Configuration( | |||||||||||||
server_index=None, server_variables=None, | ||||||||||||||
server_operation_index=None, server_operation_variables=None, | ||||||||||||||
ssl_ca_cert=None, | ||||||||||||||
*, | ||||||||||||||
debug: Optional[bool] = None | ||||||||||||||
) -> None: | ||||||||||||||
"""Constructor | ||||||||||||||
""" | ||||||||||||||
|
@@ -212,7 +215,8 @@ 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this I would suggest to at least set There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can easily add a default value for There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There may be other places with this kind of tests (they are written manually), but I didn't check. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (oh, and for the various openapi-generator/samples/openapi3/client/petstore/python/tests/test_pet_api.py Lines 6 to 11 in 793aba7
|
||||||||||||||
"""Debug switch | ||||||||||||||
""" | ||||||||||||||
|
||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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(debug=debug, expected=expected): | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TIL about unittest's |
||||||||||||||||||
c = petstore_api.Configuration(debug=debug) | ||||||||||||||||||
self.assertEqual(expected, c.debug) | ||||||||||||||||||
with self.subTest(expected=False): | ||||||||||||||||||
c = petstore_api.Configuration() | ||||||||||||||||||
self.assertEqual(expected, c.debug) | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this works a bit by chance, since Can you make the test a bit more explicit?
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. d'oh, you're right! |
||||||||||||||||||
|
||||||||||||||||||
if __name__ == '__main__': | ||||||||||||||||||
unittest.main() |
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.
There's no reason to make it a class attribute, can you move this as an instance attribute line 217?
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.
done