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

Meta, Docs: Reactive attributes that are assigned to inside __init__ need to be documented inside __init__ #1572

Closed
davep opened this issue Jan 16, 2023 · 7 comments
Assignees
Labels
documentation Improvements or additions to documentation

Comments

@davep
Copy link
Contributor

davep commented Jan 16, 2023

[Note that the subject of this issue has been modified to reflect the conclusion of this issue]

This is a bit of a meta issue that affects how the Textual docs are generated. I'm creating it as a fresh issue in our own repo as it's not obvious to me at the moment what the cause of the problem is, and I also can't recreate it in isolation right now.

This initially stems from #1564, where when I created #1570 to add docstrings for TITLE, title, SUB_TITLE and sub_title, only the upper-cased versions of those class attributes appear in the docs. I created this repo to try and isolation the problem, and see what part of the documentation pipeline might be responsible, but the result there is fine:

Screenshot 2023-01-16 at 08 19 38

So.. hence this issue. I'll track what I find here and if I find that some other tool is responsible (rather than it being a configuration issue for our docs, for example) I'll then go on to raise an issue there.

@davep davep added the documentation Improvements or additions to documentation label Jan 16, 2023
@davep davep self-assigned this Jan 16, 2023
@Textualize Textualize deleted a comment from github-actions bot Jan 16, 2023
@davep
Copy link
Contributor Author

davep commented Jan 16, 2023

Turns out it's not even something global to the Textual docs. If I add two class-attributes to Header (as a test on a small class), called FOO and foo, they come out just fine:

Screenshot 2023-01-16 at 09 33 41

@davep
Copy link
Contributor Author

davep commented Jan 16, 2023

Going back to App to see if there's something special about that and... adding FOO and foo there and they come out fine.

Screenshot 2023-01-16 at 09 40 42

So it looks like there's something special about title and sub_title.

@davep
Copy link
Contributor Author

davep commented Jan 16, 2023

I think I've finally narrowed it down. It isn't about different-case at all! It's if there's a reference to an instance attribute within __init__ that matches the class attribute, the class attribute seems to get ignored. Taking mouse_over as another test -- which is only introduced in __init__ -- no documentation appears for it if I introduce it as a class attribute.

davep added a commit to davep/textual that referenced this issue Jan 16, 2023
According to the current working of out documentation generation pipeline,
this is the way to document a reactive property that is referred to within
the owner class' __init__.

See Textualize#1572 and Textualize#1564.
@davep davep changed the title Meta, Docs: Differently-cased attributes don't appear in the docs Meta, Docs: Reactive attributes that are assigned to inside __init__ need to be documented inside __init__ Jan 16, 2023
@davep
Copy link
Contributor Author

davep commented Jan 16, 2023

So, to conclude, if a class has a reactive, that is assigned to inside the class' __init__, the docstring for the reactive should go with the assignment inside __init__ rather than its declaration on the class itself. In other words, not this:

class Foo( Widget ):

    bar = reactive( "" )
    """This is the docstring"""

    def __init__( self, baz: str ) -> None:
        self.bar = baz

But this:

class Foo( Widget ):

    bar = reactive( "" )

    def __init__( self, baz: str ) -> None:
        self.bar = baz
        """This is the docstring"""

Doing the former causes mkdocstrings(?) to see the reactive as not having documentation.

@davep davep closed this as completed Jan 16, 2023
@github-actions
Copy link

Don't forget to star the repository!

Follow @textualizeio for Textual updates.

@davep
Copy link
Contributor Author

davep commented Jan 16, 2023

Right, nope, it's still not what I discovered above; there's a further refinement to this. It isn't that the content of __init__ wins in the situations mentioned above, it's that whichever mention of the class attribute or instance attribute is seen second wins -- it's the one that needs the docstring. So, further to the two examples given above, this results in working documentation too:

class Foo( Widget ):

    def __init__( self, baz: str ) -> None:
        self.bar = baz

    bar = reactive( "" )
    """This is the docstring"""

@rodrigogiraoserrao
Copy link
Contributor

@davep as of yesterday, griffe got the PR mkdocstrings/griffe#135 that fixes this so that 0.25.6 will preserve the docstring it finds if it is the only one.

rodrigogiraoserrao added a commit that referenced this issue Apr 26, 2023
* First prototype of PB.

* Repurpose UnderlineBar.

* Factor out 'Bar' widget.

* Revert "Factor out 'Bar' widget."

This reverts commit 0bb4871.

* Add Bar widget.

* Cap progress at 100%.

* Add skeleton for the ETA label.

[skip ci]

* Add ETA display.

* Improve docstrings.

* Directly compute percentage.

* Watch percentage changes directly.

[skip ci]

* Documentation.

* Make reactive percentage private.

Instead, we create a public read-only percentage property.

* Update griffe to fix documentation issue.

Related issues: #1572, mkdocstrings/griffe#128.
Related PRs: mkdocstrings/griffe#135.

* Add example and docs.

* Address review feedback.

[skip ci]

* More documentation.

* Add tests.

* Changelog.

* More tests.

* Fix/fake tests.

* Final tweaks.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

2 participants