Skip to content

[Python] Do not use mutable default argument#4613

Closed
jirikuncar wants to merge 8 commits intoOpenAPITools:masterfrom
jirikuncar:python-mutable-default-argument
Closed

[Python] Do not use mutable default argument#4613
jirikuncar wants to merge 8 commits intoOpenAPITools:masterfrom
jirikuncar:python-mutable-default-argument

Conversation

@jirikuncar
Copy link
Contributor

@jirikuncar jirikuncar commented Nov 26, 2019

Avoid using mutable default arguments in Python.

PR checklist

  • Read the contribution guidelines.
  • If contributing template-only or documentation-only changes which will change sample output, build the project before.
  • Run the shell script(s) under ./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.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
  • File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
  • Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

--
cc @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)

@jirikuncar jirikuncar force-pushed the python-mutable-default-argument branch from 512c7c4 to 31c80b6 Compare November 26, 2019 23:16
@jirikuncar jirikuncar force-pushed the python-mutable-default-argument branch from b0dec03 to 6b058b7 Compare November 27, 2019 09:07
@jirikuncar
Copy link
Contributor Author

@spacether thank you for the review. I have cleaned up the configuration template even more based on your suggestions.

@jirikuncar jirikuncar force-pushed the python-mutable-default-argument branch from 6b058b7 to 2808b6d Compare November 28, 2019 15:34
@jirikuncar jirikuncar force-pushed the python-mutable-default-argument branch from 2808b6d to da67f9f Compare November 29, 2019 09:02
According to @spacether it is better to document only property
getter with combined signature of both getter and setter
instead of individual getters and setters.
@spacether
Copy link
Contributor

spacether commented Nov 30, 2019

@jirikuncar it sounds like there may have been a misunderstanding here. My comments were asking that you preserve the docstring information from the setters as you were deleting the types shown there. I also give a reason why the docstring for both the getter and the setter exists in the getter. In my opinion having the docstring in the getter + setter is helpful for the following reasons. As a dev I want to be able to:

  • read a getter docstring to understand what I get
  • read a setter docstring to understand what I need to set
  • use the help(ClassName) in the python terminal to read getter docstring and read what we need to get/set. In my comments I didn't ask that you delete the setter docstrings.

If you want to delete the setter docstrings I am okay with that as long as the needed information still exists in the getter docstrings.


@property
def debug(self):
"""Debug status
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a type description to this docstring
We are losing the information when we delete it from the setter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@spacether I don't get your comment.

    @property
    def debug(self):
        """The debug status.

        :param value: The debug status, True or False.
        :type: bool
        """

Copy link
Contributor

@spacether spacether Dec 3, 2019

Choose a reason for hiding this comment

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

Sorry, my mistake, I missed the remaining bool type here. This is good.

@spacether
Copy link
Contributor

@jirikuncar can you check out the Travis CI error here?
https://travis-ci.org/OpenAPITools/openapi-generator/builds/618563611?utm_source=github_status&utm_medium=notification
It looks like there are python errors in CI testing there.

@jirikuncar
Copy link
Contributor Author

I might revisit this one in the future by updating docstrings to follow PEP 257 (pydocstyle).

A minimal changed for mutable defaults has been moved to #4665.

@jirikuncar jirikuncar closed this Dec 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Comments