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

feat: type annotations for @properties #284

Open
Tracked by #1
NickCrews opened this issue Oct 3, 2023 · 5 comments
Open
Tracked by #1

feat: type annotations for @properties #284

NickCrews opened this issue Oct 3, 2023 · 5 comments
Assignees

Comments

@NickCrews
Copy link

NickCrews commented Oct 3, 2023

Thanks for all your work on this project!

Per a review comment in Ibis: ibis-project/ibis#7279 (comment)

A @property of a class is rendered without the type hint:
image

It would be nice if it WAS rendered with a type hint. IDK if this is already a feature of non-property attributes...

@machow
Copy link
Owner

machow commented Oct 6, 2023

Thanks for raising, it seems like the renderer has everything it needs to render annotation types, but it's a bit tricky to figure out these aspects of generating attributes:

  • Attributes can be documented individually, or via the numpydoc Attributes section.
  • Some doc sites summarize attributes in tables, while others like to document them individually (like methods).
    • i.e. a property's docstring can be a full fledged numpydoc docstring, with sections. This wouldn't really fit a table.

As a result, quartodoc handles attributes using these behaviors:

  • If there is a attributes section in the class docstring, render it like a parameter table (including types)
  • otherwise, render the attributes table like the methods summary table (no types)

Minimal reproducible example

Does this look like I'm reproducing the situation?

test_func.py

class Table:
    @property
    def x(self) -> int:
        """The x attribute"""
        return 1

rendering doc

from quartodoc import Auto, blueprint, MdRenderer

bp = blueprint(Auto(name = "test_func.Table"))
print(MdRenderer().render(bp))
# test_func.Table { #test_func.Table }

`Table()`



## Attributes

| Name | Description |
| --- | --- |
| [x](#test_func.Table.x) | The x attribute |

Fixing this particular issue in ibis

Option 1: add attributes section

WDYT of adding an attributes section to your docstring for now? It currently, requires repeating the description (will open issue upstream in griffe), but does fetch the type info automatically from the property.

Here's an example

class Table:
    """
    Attributes
    ----------
    x
        The x attribute
    """

    @property
    def x(self) -> int:
        """The x attribute"""
        return 1

Option 2: document attributes more like they're methods

In this case, we'd have the attributes summary link to a fully rendered docstring for each attribute, as if they're methods. If this seems like a better option, let me know. I think we'd need to add an option to enable this style of documentation on classes / modules.

@NickCrews
Copy link
Author

Thanks for the detailed explanation!

Does this look like I'm reproducing the situation?

Yes I think you have it.

Whatever the rendered output, I want it to come from the logic of

  • look through the class's Attributes section for names, types, and descriptions
  • look through all @propertys for names types and descriptions
  • Whenever something is found in both, error.
  • Union these two sets together.
    I think ideally we should support both sources: Some attributes are dynamically generated, so we need the Attributes section for these. The rest of the time, I want the documentation to live next to the implementation in a docstring.

Then, once we have the list of annotations, we need to choose how to render them. My ideal would be

  • have a config option for rendering each attribute in a full section, similar to methods. In simple cases like this example, I wouldn't want this, I'd just want the summary table. Maybe the default depends on if each attribute just has a oneliner docstring then you can just have the table. If ANY docstring is multiline then each attribute renders individually.
  • In the summary table, add a column for the type. Possibly add a config option for this in case the type is really long and would look ugly, but I would probably say just always render it.

CC @cpcloud @lostmygithubaccount @jcrist @gforsyth if they have any opinions

@machow
Copy link
Owner

machow commented Oct 10, 2023

Thanks for weighing in, this is super helpful. The suggestion on rendering makes a lot of sense.

Just to recap, it sounds like the option to render like methods (let's say expand_attributes for now) might look like:

  • True: summary table looks like methods now (name, short description), and links to longer attribute doc
  • False: summary table looks like parameters table (name, type, short description)

WDYT of that approach? (I'd lean on punting on any automated toggling between behaviors for now, but it seems useful to implement down the road, based on whether attribute docs are one-liners).

Minor detail on documenting attributes

Some attributes are dynamically generated, so we need the Attributes section for these. The rest of the time, I want the documentation to live next to the implementation in a docstring.

You should be able to document any class attribute, even a dynamically created one, using this syntax:

class SomeClass:
    x: int
    """The x value-less variable, whose value might be set on instantiation"""

@NickCrews
Copy link
Author

That sounds good! Instead of a true/false though for the option, I would name it something like attributes and have the options be table_and_sections or table_only. This feels more interpretable to me, and leaves the door open to add more options later.

Honestly if you really want to kick the ball down the road, if you just implemented the table_only version and made that the default then that would solve the problem that motivated this issue. In Ibis we haven't yet needed a full section for documenting attributes. Then you could hold off on exposing this as config until someone else files another feature request, and then you'll have even more experience/knowledge so the solution then could be even more refined. Either way though this discussion was useful!

I agree that automated choosing can happen later.

Minor detail on documenting attributes

Oh yeah, that's great, thanks for pointing that out!

@pawamoy
Copy link

pawamoy commented Oct 22, 2023

My recommendation (which is not documented anywhere yet) is to always document each attribute and property separately. Attributes sections in the parent docstring shouldn't be preferred, as they can just be generated automatically by iterating over the attributes to pick up their summary and type. Since each attribute has a full docstring, it can have its own heading+permalink+toc entry, and generated tables can link to it.

As stated by @machow, even dynamic attributes can be given a docstring thanks to this syntax:

class A:
    attr_a: int
    """Docstring."""

    # or even in the __init__ method
    def __init__(self):
        self.attr_b: float
        """Docstring."""

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Backlog
Development

No branches or pull requests

3 participants