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

Improve @property documentation #39

Open
Tracked by #492
leandro-lucarella-frequenz opened this issue Oct 19, 2022 · 8 comments
Open
Tracked by #492

Improve @property documentation #39

leandro-lucarella-frequenz opened this issue Oct 19, 2022 · 8 comments
Assignees
Labels
part:docs Affects the documentation type:enhancement New feature or enhancement visitble to users
Milestone

Comments

@leandro-lucarella-frequenz
Copy link
Contributor

leandro-lucarella-frequenz commented Oct 19, 2022

What's needed?

Right now we are documenting properties as if they were functions (a description and a Return section are both required), but they are clearly not.

Google style says properties should be documented as attributes (in the class documentation) but darglint doesn't support this.

Proposed solution

  • Properties:
    • @properties are rendered as function (with a special mark for being a property), but this is planned to be improved in the future to be shown as attributes (see Use MkDocs to build the documentation frequenz-channels-python#25 (comment)).

    • Removing documentation from the function and using Attributes: instead somewhat works, but you only get a table in the Class documentation, attributes are not shown in the right ToC, which makes them hard to discover IMHO.

    • The best approach to @properties, that works well with pydocstyle and darglint seems to be the following:

      class AClass:
          """A class."""
      
          @property
          def a_prop(self) -> int:
              """Get the a prop.
      
              Returns:
                  The a prop of the class.
              """
              return 1
      
          @a_prop.setter
          def a_prop(self, a: int) -> None:
              pass

      image

      The only downside is properties are still listed as functions, not attributes (but this will change, so all good with this), and there is redundancy in the documentation (basically we need to document it twice, one for the description and one for the Returns:. mkdocstrings doesn't show the type if Returns: is not specified, and if the description is not included, pydocstyle complains.

So far the only option seems to be fixing the issue in the upstream darglint tool.

Use cases

No response

Alternatives and workarounds

We tried to use # noqa as suggested in the darglint issue but according to @jakub-toptal it doesn't work:

Before:

@property
def microgrid_api_client(self) -> MicrogridApiClient:
    """Get MicrogridApiClient.

    Returns:
        api client
    """
    return self._api

After:

    @property  # noqa
    def microgrid_api_client(self) -> MicrogridApiClient:  # noqa
        """Get MicrogridApiClient."""  # noqa
        return self._api  # noqa

Still fails with darglint code: DAR201

Additional context

There is a darglint issue for this, and at some point we might want to give it a shot to improve property handling in darglint:

@leandro-lucarella-frequenz leandro-lucarella-frequenz added part:docs Affects the documentation priority:low This should be addressed only if there is nothing else on the table type:enhancement New feature or enhancement visitble to users labels Oct 19, 2022
@leandro-lucarella-frequenz leandro-lucarella-frequenz added this to the v0.12.0 milestone Oct 25, 2022
@leandro-lucarella-frequenz
Copy link
Contributor Author

We will generate the documentation with mkdocstrings, which also doesn't support properties super gracefully. It doesn't work well with Attributes: either.

To produce a decent result, the minimum documentation needed is:

class AClass:
    """A class."""

    @property
    def a_prop(self) -> int:
        """
        Returns:
            The a prop of the class.
        """
        return 1

    @a_prop.setter
    def a_prop(self, a: int) -> None:
        pass

(note the Returns: can be next to the """, otherwise it doesn't work either, and if the Returns: is not included, then the type of the property is not included either)

It produces this results:

image

(so it's not ideal that it is listed as a function, but it is nicely annotated as a writable property)

But pydocstyle complains about this:

in private method `a_prop`:
        D205: 1 blank line required between summary line and description (found 0)
in private method `a_prop`:
        D400: First line should end with a period (not ':')
in private method `a_prop`:
        D401: First line should be in imperative mood (perhaps 'Return', not 'Returns')

So it basically demands a text besides the Returns: section. darglint doesn't complain with this.

So we can explore if we could disable those pydocstyle somehow, or just live with the redundancy of having the properties documented twice. This passes all tests:

class AClass:
    """A class."""

    @property
    def a_prop(self) -> int:
        """Get the a prop.

        Returns:
            The a prop of the class.
        """
        return 1

    @a_prop.setter
    def a_prop(self, a: int) -> None:
        pass

@leandro-lucarella-frequenz
Copy link
Contributor Author

Given the amount of work that could be involved in making sure all the tools we use work well with one particular way to documenting properties, I'm starting to lean more towards to just drop this issue and bite the bullet.

@leandro-lucarella-frequenz
Copy link
Contributor Author

Adding the proposed solution to the issue body.

@leandro-lucarella-frequenz
Copy link
Contributor Author

Doing some tests with pydoclint / pydocstyle / mkdocstrings it seems like now we should be able to just write:

@property
def prop(self) -> T:
    """The property."""

And it should work fine, so this might get a lot easier soon.

@leandro-lucarella-frequenz leandro-lucarella-frequenz removed their assignment Jun 2, 2023
@llucax
Copy link
Contributor

llucax commented Jul 6, 2023

Blocking on adopting pydoclint

@llucax llucax added the status:blocked Other issues must be resolved before this can be worked on label Jul 6, 2023
@llucax llucax modified the milestones: v0.23.0, v1.0.0 Jul 11, 2023
@llucax llucax modified the milestones: v1.0.0-rc1, v1.0.0-rc2 Sep 8, 2023
@llucax llucax moved this from In progress to To do in Python SDK Roadmap Oct 5, 2023
@llucax llucax modified the milestones: v1.0.0-rc2, v1.0.0-rc3 Oct 10, 2023
@llucax llucax modified the milestones: v1.0.0-rc3, v1.0.0-rc4 Nov 8, 2023
@llucax llucax modified the milestones: v1.0.0-rc4, v1.0.0 Nov 16, 2023
@llucax llucax moved this from To do to In progress in Python SDK Roadmap Dec 5, 2023
@llucax llucax modified the milestones: v1.0.0, v1.0.0-rc6 Feb 5, 2024
@llucax llucax modified the milestones: v1.0.0-rc6, v1.0.0-rc7 Mar 26, 2024
@llucax llucax modified the milestones: v1.0.0-rc800, v1.0.0 Jul 29, 2024
@shsms shsms modified the milestones: v1.0.0-rc800, v1.0.0-rc900 Aug 22, 2024
@llucax llucax modified the milestones: v1.0.0-rc900, 1.0.0-rc1000 Sep 2, 2024
@llucax llucax modified the milestones: v1.0.0-rc1000, v1.0.0-rc1100 Oct 21, 2024
@llucax llucax modified the milestones: v1.0.0-rc1100, v1.0.0-rc1200 Nov 11, 2024
@llucax llucax modified the milestones: v1.0.0-rc1200, v1.0.0-rc1400 Nov 20, 2024
@llucax llucax modified the milestones: v1.0.0-rc1400, v1.0.0-rc1500 Nov 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
part:docs Affects the documentation type:enhancement New feature or enhancement visitble to users
Projects
Status: In progress
Development

No branches or pull requests

6 participants