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

Widget documentation sweep #1909

Merged
merged 95 commits into from
Mar 1, 2023
Merged

Widget documentation sweep #1909

merged 95 commits into from
Mar 1, 2023

Conversation

davep
Copy link
Contributor

@davep davep commented Mar 1, 2023

A wander through various widgets, adding missing docstrings, trying to make others better fit in with our standards, and with the occasional type hint tweak where necessary.

See #1811.

Also delivers on the scroll_ docstring promise made in #1902.


NOTE: Part of this PR also experiments with a slight change to the layout of the API docs; currently only with the Tree. Rather than document Tree and TreeNode as two very different things, it documents them both under Tree. This means that it's then possible to document some of the other public-facing types without the reader having to try and guess which might relate to Tree, which to TreeNode, and which to both (and then guess which will be living where).

@davep davep added documentation Improvements or additions to documentation Task labels Mar 1, 2023
@davep davep self-assigned this Mar 1, 2023
@davep davep linked an issue Mar 1, 2023 that may be closed by this pull request
9 tasks
davep added 11 commits March 1, 2023 10:39
While it's arguably not the most useful widget, it's in the code, exported,
and can be seen by people; I feel it should be in the documentation. I've
ensured that it also links to Placeholder given it sort of is a Placeholder
and so people might arrive at it first and it makes sense to redirect them
to something more comprehensive.
@davep
Copy link
Contributor Author

davep commented Mar 1, 2023

Managed to typo an issue reference in the last set of changes relating to the scroll_ docstring updates, resulting in a future issues/PR/something (hello everyone reading #1920) possibly getting a bunch of unexpected references, when I meant to reference #1902.

(╯°□°)╯︵ ┻━┻

@davep davep marked this pull request as ready for review March 1, 2023 15:15
@davep davep requested review from willmcgugan, rodrigogiraoserrao and darrenburns and removed request for willmcgugan and rodrigogiraoserrao March 1, 2023 15:16
davep added 2 commits March 1, 2023 16:07
Tidying up after an on-forge merge conflict resolution that of course ended
up bypassing black.
force: Force scrolling even when prohibited by overflow styling. Defaults to `False`.
x: X coordinate (column) to scroll to, or `None` for no change.
y: Y coordinate (row) to scroll to, or `None` for no change.
animate: Animate to new scroll position.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the sentence "Animate to new scroll position" is a bit cryptic.
Do you think something like "Whether to animate the scrolling (or otherwise just jump to the new position)" or something along those lines is an improvement?
Do you have a better alternative?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to have improvements made to it; I was just cleaning up prior docs really.

src/textual/widgets/_header.py Outdated Show resolved Hide resolved
src/textual/widgets/_header.py Outdated Show resolved Hide resolved
src/textual/widgets/_header.py Outdated Show resolved Hide resolved
"""Initialise the header widget.

Args:
show_clock: ``True`` if the clock should be shown on the right of the header.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a difference between surrounding True with a pair of backticks versus two pairs of backticks? I have seen both and haven't understood the difference yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question; I was asking myself the same thing as I was going through the docstrings and seeing a difference. I tested it and from what I can tell there's zero difference; both have the same effect (my uninformed guess here would be it's mkdocstrings or something supporting a couple of other doc systems' styles).

src/textual/widgets/_pretty.py Outdated Show resolved Hide resolved
@@ -27,7 +30,7 @@ class TextLog(ScrollView, can_focus=True):
}
"""

max_lines: var[int | None] = var(None)
max_lines: var[int | None] = var[Optional[int]](None)
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as my understanding goes, var is just an "alias" for a quick way to create a reactive attribute with some parameters in pre-defined values.
Would it be worth typing max_lines as max_lines: reactive[int | None] = var[Optional[int]](None)?
(This was not a rhetoric question.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What benefit do we get from that? A downside I can see is that it might be confusing to a reader of the code, seeing two "different" types like that.

src/textual/widgets/_text_log.py Show resolved Hide resolved
src/textual/widgets/_tree.py Outdated Show resolved Hide resolved
src/textual/widgets/_tree.py Outdated Show resolved Hide resolved
davep and others added 6 commits March 1, 2023 16:16
Co-authored-by: Rodrigo Girão Serrão <[email protected]>
Co-authored-by: Rodrigo Girão Serrão <[email protected]>
Co-authored-by: Rodrigo Girão Serrão <[email protected]>
Co-authored-by: Rodrigo Girão Serrão <[email protected]>
Co-authored-by: Rodrigo Girão Serrão <[email protected]>
Co-authored-by: Rodrigo Girão Serrão <[email protected]>
@davep davep merged commit 086c7d6 into Textualize:main Mar 1, 2023
@davep davep deleted the widget-doc-sweep branch March 1, 2023 16:43
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 Task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add missing docstrings to a number of widgets
2 participants