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

docs: add main to ops api reference #1273

Merged
merged 4 commits into from
Jun 27, 2024

Conversation

IronCore864
Copy link
Contributor

@IronCore864 IronCore864 commented Jun 25, 2024

The docstring of main is missing from RTD, this adds it back.

According to Dora, it would be nice to link to main in here.

I don't have a very good solution to add it, did some testing, this is the best I can do, preview here.

Tried to put it in front of the first entry of the ops module but then the indentation for following items are wrong, since they are handled automatically by autodoc.

Also tried to add only the function ops.main.main instead of the module, in this way we can put it in front of ops.ActionEvent, but it is still before the big paragraph that comes from __init__.py, which looks even worse.

So, the current preview looks the best, although having main at the bottom seems not so good.

@tmihoc
Copy link
Member

tmihoc commented Jun 25, 2024

Oh, I see now what you mean by it being rendered at the bottom:

image

So, before we had a gap but things looked fine. Now we've filled the gap but the placement is not ideal. Which one would I rather have? Definitely the latter. If ops.main is a construct in Ops, I expect to be able to find it in the search and to be able to link to it. So, I think this PR is in the right direction. Approved from my part, and thanks!

PS People do not consume reference start to finish but rather jump to it as necessary. I wouldn't worry about the placement too much. (I mean, if we can fix it, great, but it's not a deal-breaker for me.)

PPS For consistency with the other titles in the Navigation, the title should be just main (not ops.main).

Copy link
Collaborator

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

This seems fine to me, thanks! Not perfect placement, but seems like a good start!

I guess there's no way to insert ops.main into the list of autodoc'd members?

@IronCore864
Copy link
Contributor Author

I guess there's no way to insert ops.main into the list of autodoc'd members?

I've also tried to do this but failed, no matter what I did, it wouldn't show up in the members...

@dimaqq
Copy link
Contributor

dimaqq commented Jun 26, 2024

I too was wondering why reference for main was missing.

Thank you for adding it :)

@tonyandrewmeyer
Copy link
Contributor

@IronCore864 if you rebase this the tests should pass now.

@IronCore864 IronCore864 merged commit 1a11311 into canonical:main Jun 27, 2024
27 checks passed
@IronCore864 IronCore864 deleted the add-main-to-rtd branch June 27, 2024 02:47
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.

5 participants