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

Export types & doc improvements #2329

Merged
merged 7 commits into from
May 2, 2023
Merged

Export types & doc improvements #2329

merged 7 commits into from
May 2, 2023

Conversation

rodrigogiraoserrao
Copy link
Contributor

@rodrigogiraoserrao rodrigogiraoserrao commented Apr 19, 2023

This will close #2168 when it's done.

I did a pass on the App API page and looked for types that are used in signatures and that aren't documented.
This is the type of change I suggest.

If everyone is ok with this type of changes, I will go over the remainder of the API pages.

We still need to figure out how to document type aliases without duplicating their definition.

@willmcgugan
Copy link
Collaborator

Looks good. Please add screenshots when its ready!

@rodrigogiraoserrao
Copy link
Contributor Author

As of bbde665, this PR exports almost everything that the docs link to via argument types, return types, and exceptions raised.

To accommodate this, the API section grew 4 pages:

  • Errors (errors.py)
  • Filter (filter.py)
  • Scrollbar (scrollbar.py)
  • Types (new file types.py)

The first three pages contained 2+ objects that were referenced in other API pages so I thought it was best to add the whole API page.

Questions for @willmcgugan:

  • I have no idea what rule(s) we've been following to determine what API pages to include, so I'm not sure it's ok to add the API pages errors, filter, and scrollbar.
  • The page "Types" was named like that because I thought I'd end up only documenting type aliases there. As it turns out, there are a couple of odd exceptions, literals, type variables, and classes, that come from private modules but that can be used/returned by/raised by public methods/attributes. I feel tempted to rename this API page to "Miscellaneous" but still keep it at the end of the API menu.
  • I exported RenderStyles via the page "Types"; should I add the new API page "Styles" for styles.py?

CC @davep because you seem to have sensible ideas and suggestions when it comes to the docs :P

@rodrigogiraoserrao rodrigogiraoserrao marked this pull request as ready for review April 21, 2023 17:24
Copy link
Collaborator

@willmcgugan willmcgugan left a comment

Choose a reason for hiding this comment

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

You've removed the Returns: sections I intentionally put it.

Without them I couldn't get the docs to actually link to the return values of properties. Unless you have found a way of making that work automatically, can you restore the Returns:

src/textual/app.py Outdated Show resolved Hide resolved
src/textual/app.py Outdated Show resolved Hide resolved
src/textual/app.py Outdated Show resolved Hide resolved
src/textual/message_pump.py Show resolved Hide resolved
@davep
Copy link
Contributor

davep commented Apr 24, 2023

All looking good to me; although it does raise one thing that overlaps with this quite a bit. This might be best as another issue but perhaps some of this can be rolled in here as it is about types and linking.

When I see something like this:

[VideoPlayer][textual.widgets._video_player.VideoPlayer]

I feel it should really be like this:

[`VideoPlayer`][textual.widgets._video_player.VideoPlayer]

as the result in the docs does give the type name a slightly different, and I think beneficial, styling.

Strong feelings either way, and scope for this or a followup PR?

Copy link
Contributor

@davep davep left a comment

Choose a reason for hiding this comment

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

Approving within the bounds of Will's requested changes.

@rodrigogiraoserrao
Copy link
Contributor Author

Strong feelings either way, and scope for this or a followup PR?

I feel strongly that you are right and that mentions to code objects/entities should be styled with backticks. I'll add those in.
Feel free to open the issue if you think it's a good idea, but I'll tackle that issue here.

@rodrigogiraoserrao rodrigogiraoserrao changed the title Export types used in app.py Export types & doc improvements Apr 24, 2023
We need to have explicit 'Returns:' sections in properties if we want to link to the return type while mkdocstrings/python#65 is open.
@rodrigogiraoserrao
Copy link
Contributor Author

Attributes now get their source, after I reverted a change from a couple of months ago:

Untitled

The issue is that ALL attributes now show their first assignment and this can look a bit odd in some places, e.g., here are the two reactive attributes for Button:

Untitled

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document custom types used and actually show their definition
3 participants