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

Fix documentation warnings #3425

Closed
wants to merge 6 commits into from

Conversation

guy-av
Copy link
Contributor

@guy-av guy-av commented Sep 28, 2023

Please review the following checklist.

  • Updated documentation
  • For some reason textual.app.App.compose() doesn't seem to appear in the reference section of the docs hence breaking a specific link on the App basics page.

Resolves #2699

@rodrigogiraoserrao
Copy link
Contributor

The issue with the compose method is that it is excluded from the docs, but if it is linked from elsewhere, we either link to something else or we make sure that App.compose is documented as well.
Look at the api documentation for Widget, where we override some methods that are otherwise excluded via the documentation configuration.

@guy-av guy-av force-pushed the fix/documentation-warnings branch from 9ea9bd3 to e9a67c0 Compare October 4, 2023 21:05
@guy-av
Copy link
Contributor Author

guy-av commented Oct 4, 2023

@davep made sure the filters in api/app.md match those of the widgets, filtering out only __init__ and private members. Will you take another look please?

@@ -165,6 +165,7 @@ nav:
- "widgets/tabbed_content.md"
- "widgets/tabs.md"
- "widgets/text_area.md"
- "widgets/toast.md"
Copy link
Contributor

Choose a reason for hiding this comment

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

My memory is hazy, but I'm sure we left toast out of the widget list as it's not really a "public-facing" widget; it's more of an internal support widget for the default implementation of App.notify.

@@ -174,6 +175,7 @@ nav:
- "api/color.md"
- "api/command.md"
- "api/containers.md"
- "api/content_switcher.md"
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that ContentSwitcher is within the widgets, I'm wondering why there is an api/content_switcher.md at all.

@davep
Copy link
Contributor

davep commented Oct 4, 2023

made sure the filters in api/app.md match those of the widgets, filtering out only init and private members. Will you take another look please?

@guy-av Forgive me, I may have missed some context here. Another look? Is this something I've looked at before?

@guy-av
Copy link
Contributor Author

guy-av commented Oct 4, 2023

made sure the filters in api/app.md match those of the widgets, filtering out only init and private members. Will you take another look please?

@guy-av Forgive me, I may have missed some context here. Another look? Is this something I've looked at before?

Actually didn't mean to tag you 😅 sorry about that.
Thanks for reviewing though!

@@ -825,7 +825,7 @@ def get_option(self, option_id: str) -> Option:
"""
return self.get_option_at_index(self.get_option_index(option_id))

def get_option_index(self, option_id):
def get_option_index(self, option_id: str):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we have a return type here.

@rodrigogiraoserrao
Copy link
Contributor

I'm closing this as it got stale and was superseded by the PR linked above.

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.

Documentation warnings deserving attention
4 participants