Skip to content
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

Unify properties getter, setters and deleters under a single documentation entry #587

Open
tristanlatr opened this issue May 22, 2022 · 3 comments · May be fixed by #640
Open

Unify properties getter, setters and deleters under a single documentation entry #587

tristanlatr opened this issue May 22, 2022 · 3 comments · May be fixed by #640
Labels
astbuilder A substantive change is required in the astbuilder flow in order to fix this issue
Milestone

Comments

@tristanlatr
Copy link
Contributor

It would good to see all the information about a property inside a single documentation entry.

The main issue is what to do if several object have docstrings assigned to ? Should they be merge them together? Should we display them in separate boxes?

Or maybe we should log a warning an use only the getter docstring. This would be the most simple solution and also comply with the default behavior of help(), but we’ve been using docstring on property setter in our demo. So we should probably look at a way of still supporting that.

Anyway the information whether this property is writable or deletable or read only can be added as extra_info.

@tristanlatr tristanlatr added the astbuilder A substantive change is required in the astbuilder flow in order to fix this issue label May 22, 2022
@tristanlatr tristanlatr added this to the 22.x milestone May 22, 2022
@tristanlatr
Copy link
Contributor Author

This is not valid python, hopefully, so it makes things a bit simpler.

class Person:
    _name=None
    @property
    def name(self):
        return self._name
@Person.name.setter
def name(self, value):
    self._name = value

@tristanlatr
Copy link
Contributor Author

But this is valid python:

class A:
    _data=None
    @property
    def data(self):
        if self._data is None:
            self._data = Data(self)
        return self._data

The thing is the current handling of properties makes us blind to any instance attributes declared in property getters, i.e. we won't not visit the self._data = Data(self) statement. And _data will be wrongly categorized as class variable.

So I think the manner to handle property is to first of all create Function for everything, this will allow us to walk the body of the property getters and have the "correct" representation. Then in a post-processor, we machup property functions into an Attribute.

tristanlatr added a commit that referenced this issue Aug 5, 2022
…deleters under a single documentation entry.
@tristanlatr tristanlatr linked a pull request Aug 5, 2022 that will close this issue
@tristanlatr
Copy link
Contributor Author

So turns out proper support for properties is a bit more complex that I would imagine:

So we can end up with code like:

    class BaseClass(object):
        def __init__(self):
            self._spam = 5
        @property
        def spam(self):
            """BaseClass.getter"""
            return self._spam
        @spam.setter
        def spam(self, value):
            self._spam = value
        @spam.deleter
        def spam(self):
            del self._spam
    class SubClass(BaseClass):
        @BaseClass.spam.getter
        def spam(self): # this overrides the getter only of the property and use the inherited functions for the setter and deleter.
            """SubClass.getter"""
            ...

This brings other considerations for pydoctor, like how to present the fact that some parts of the property methods are inherited and some parts not? And this complexifies the AST building part.

The simplest thing to do would be to always create a property if some parts of a property are defined, show all property methods, but say which are inherited.

@tristanlatr tristanlatr modified the milestones: 22.x, 23.x Jan 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astbuilder A substantive change is required in the astbuilder flow in order to fix this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant