-
Notifications
You must be signed in to change notification settings - Fork 609
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: Reorganize reference v2 #7159
Conversation
f8c1319
to
6f66df5
Compare
6f66df5
to
216a2f1
Compare
I just fixed those broken links, but this is ready for review! It is a little hefty, so I appreciate the effort! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you can remove the changes to the backend layout this LGTM!
216a2f1
to
83c9e5d
Compare
I am getting warning: (but still builds)
is this to be expected or did I break something? |
I've never seen this before working w/ Quarto I will give a drive-by comment while I'm here that I don't agree w/ hiding the backend docs within the API docs. I understand that it makes sense, as they are API docs to a large extent, but it's useful for users IMO to easily see the list of available backends at a top-level and we can do more platform-specific content in those pages going forward |
Ah I think my latest screenshot was confusing. The backend stuff is NOT underneath the rest of the API stuff, it's in its own top level tab as it was before. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM!
@lostmygithubaccount Can you give a review?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM -- can ignore that nit but if it's easy to fix that'd be nice for consistency
I will push those two fixes! |
83c9e5d
to
31db5ab
Compare
|
31db5ab
to
c0db0f5
Compare
docs/_quarto.yml
Outdated
@@ -111,6 +111,7 @@ website: | |||
collapse-level: 2 | |||
contents: | |||
- support_matrix.qmd | |||
- reference/connection.qmd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move this to be only in the API docs? I feel like we should keep the backend tab to only contain the support matrix and the backends and nothing else. Otherwise it looks cluttered to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. Under what section and what should the page be named?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add it this in a follow up PR? In that PR I think you remove this section and let the API docs show up as they usually do under the Reference
tab.
RE
Just a heads up, in quartodoc you can set Here are the docs on setting options: https://machow.github.io/quartodoc/get-started/basic-content.html#setting-default-options |
docs/_quarto.yml
Outdated
- name: random | ||
dynamic: true | ||
- name: range_window | ||
- name: set_backend |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually can you add these in a follow up PR? It's hard to see the structural changes here without building the docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not quite sure what you mean by "these"? In my latest push I didn't move anything to underneath the Backends tab. Now just the "top-level" section only contains connection stuff and is renamed "Connection API". Hopefully this is clear enough.
I wish quarto had a preview feature for PRs like this, like readthedocs does... :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, do you mean just remove commit b3429e9 from this PR and follow up with that later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry! By "these" I mean the set_backend
/get_backend
/connect
methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assumed b3429e9 so, so I removed it from the commit stack.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, now I see your clarification, thanks! I think now your concern is rendered useless, as now these methods aren't getting moved. Everything else is just getting moved out, and this section is getting renamed.
This was failing to render in the reference sections of the docs.
c0db0f5
to
14d5069
Compare
14d5069
to
d174964
Compare
b3429e9
to
554c73a
Compare
I think this is good to go in now! I moved Connections API to its own section, and fixed a few failing references. |
Money, thank you so much for finishing this up and getting it merged! |
Now the signatures appear as `ibis.random()` instead of just `random()`. Per ibis-project#7159 (comment) Before, quartodoc wasn't even respecting this option, because it was incorrectly definied at the wrong nesting level. I had to move the option out of the "member_options" section for any change to occur.
Now the signatures appear as `ibis.random()` instead of just `random()`. Per ibis-project#7159 (comment) Before, quartodoc wasn't even respecting this option, because it was incorrectly definied at the wrong nesting level. I had to move the option out of the "member_options" section for any change to occur.
Now the signatures appear as `ibis.random()` instead of just `random()`. Per #7159 (comment) Before, quartodoc wasn't even respecting this option, because it was incorrectly definied at the wrong nesting level. I had to move the option out of the "member_options" section for any change to occur.
This is a followup per #7141
In addition to these high-level changes, also a few tweaks:
import ibis
, then we are all good. But due to the reordering I did, this no longer happened here, so I had to add this import. At a higher-level, this isn't great, because it means different docstrings share state, and that depends on their ordering in a page. yuck. But maybe not worth it to fix. Possibly we could del all existing variables with some prelude lines, similar to what is done in docs: add prelude lines to all code samples #7158, but IDK this might all be overkill.Things that I think should still happen in a followup PR:
now()
in the temporal section, I wish the function signature wasibis.now()
.ibis.random()
instead ofibis.expr.api.random()
.