Uh oh!
There was an error while loading. Please reload this page.
[python] add method to set/get default configuration - #5315
Conversation
👍 Thanks for opening this issue! The team will review the labels and make any necessary changes. |
eae7d7a to
e4c6e2bCompareThere was a problem hiding this comment.
Why copy it vs just returning the instance?
The CI tests are failing when trying to deepcopy the logger.
There was a problem hiding this comment.
How about returning a new Configuration instance which uses the same instantiation inputs as the default instance?
There was a problem hiding this comment.
Yes, deepcopy doesn't work. Here as in the previous implementation a copy of default configuration is returned.
The CI tests failed because a remote endpoint returned 404 error on add_pet method. I don't know why, I'll try to run it again.
There was a problem hiding this comment.
Tests passed. I guess the original intention was to return a copy which looks like a new object but with different default values. Maybe I should name the method new_instance instead of get_instance to make it more understandable... ?
There was a problem hiding this comment.
I changed method name to new_instance().
There was a problem hiding this comment.
new_instance is good.
How do you feel about get_default_copy as a name?
There was a problem hiding this comment.
Updated, both versions look good to me.
867e860 to
70a5854CompareUh oh!
There was an error while loading. Please reload this page.
spacether
left a comment
There was a problem hiding this comment.
The tests here look good.
It may also make sense to add a test to make sure that the logger has a different id but I am fine without it.
I have some questions and suggestions about how to implement copy.
| self.client_side_validation = True | ||
| @classmethod | ||
| def copy(cls, source): |
There was a problem hiding this comment.
How about changing this to:__deepcopy__(self, memodict={})
That way you control how everything can be copied. For the logger only you can then make a new logger with the same properties as the old one. All other properties can be deep copied. You can use dir(self) or self.__dict__ to iterate over all of self's properties.
If you do it this way, then in new_instance you can return copy.deepcopy(cls._default)
There was a problem hiding this comment.
Yeah, I'd totally forgotten about this possibility. Thanks.
spacether
left a comment
There was a problem hiding this comment.
Good progress, please see my other comments
| """ | ||
| if cls._default is not None: | ||
| return copy.deepcopy(cls._default) | ||
| return Configuration() |
There was a problem hiding this comment.
How about returning None if the default copy is not present?
Then we can do truthy/none checks with the result.
This lets our users see and handle the case when a default is unset.
Or if we want to always return an instance how about naming it
default_copy_or_new_instance?
There was a problem hiding this comment.
I think get_default_copy is good enough for this usage.
| cookie=None, pool_threads=1): | ||
| if configuration is None: | ||
| configuration = Configuration() | ||
| configuration = Configuration.get_default_copy() |
There was a problem hiding this comment.
How about?
default_copy = Configuration.get_default_copy()
configuration = Configuration() if default_copy is None else default_copy
tomplus
commented
Feb 23, 2020
Thank you. |
* [python] add method to set/get default configuration * [python] change method name and fix handling api_key * python: using modified deepcopy to set/get default configuration * python: update samples * python: update samples
As a follow up the discussion #4485 (comment) I'd like to recovery an option to set default configuration. I prepared very simple solution which provides the same API as we had before.
Thanks for reviewing it.
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.shif 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.@taxpon (2017/07) @frol (2017/07) @mbohlool (2017/07) @cbornet (2017/09) @kenjones-cisco (2017/11) @tomplus (2018/10) @Jyhess (2019/01) @slash-arun (2019/11) @spacether (2019/11)