-
Notifications
You must be signed in to change notification settings - Fork 21
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
feat: Allow addings additional options to sidebar
#370
Conversation
Thanks for taking the time to work on this! I'm onboard with either approach, so whatever you think is better! It seems like an advantage of having a single (RE quarto using a edit: I think I understand the contents piece more now and am big into it |
Here are some relevant issues I found: |
* Use `file` to specify the sidebar file * Use "{{ contents }}" as a placeholder for quartodoc's contents anywhere in `quartodoc.sidebar.contents`.
@machow I've reworked the PR to use a dictionary for One place I could use your expertise in the review is making sure there aren't unwanted upstream consequences. I think I've caught everywhere that |
if self.sidebar: | ||
_log.info(f"Writing sidebar yaml to {self.sidebar}") | ||
_log.info(f"Writing sidebar yaml to {self.sidebar['file']}") | ||
self.write_sidebar(blueprint) |
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, I think I've convinced myself that everything is all good. Here's where we decide whether or not to write the sidebar, by which point self.sidebar
is guaranteed to be a dictionary containing at least a file
item (in Builder.__init__
just above in the diff).
sidebar_options
for configuring the sidebarsidebar
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.
Thanks, this looks really great! I left a small nit asking whether we could pull out the splice function onto the class or as a top-level function. It took a second to get the gist of what was happening, but as a general function it seems like it'd be quick to pick up on what's happening!
quartodoc/autosummary.py
Outdated
if not isinstance(sidebar["contents"], list): | ||
raise TypeError("`sidebar.contents` must be a list") | ||
|
||
def splice_contents_recursive(sidebar, contents): |
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.
Do you mind pulling this into a staticmethod on the class (or a function outside the class)? It feels like if it were called _insert_contents(x: dict | list, contents: ?)
or similar, the idea of a utility function putting contents where a sentinel string is is a bit easier to think about outside of the context of the sidebar.
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.
@machow No problem! I refactored splice_contents_recursive()
into a top-level _insert_contents()
function, generalized the names a bit, and added a docstring.
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #370 +/- ##
==========================================
+ Coverage 88.72% 88.78% +0.05%
==========================================
Files 37 37
Lines 2963 3004 +41
==========================================
+ Hits 2629 2667 +38
- Misses 334 337 +3 ☔ View full report in Codecov by Sentry. |
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.
Thanks for making the changes -- hope it's okay, I moved some of the advanced sidebar piece into its own section on the sidebar page
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.
Thanks for making the changes -- hope it's okay, I moved some of the advanced sidebar piece into its own section on the sidebar page
Currently, (AFAICT) it's not possible to configure the sidebar generated by quartodoc.
This PR adds support using a dictionary for
quartodoc.sidebar
which is then passed directly to Quarto, after filling in theid
andcontents
created by quartodoc.A string
quartodoc.sidebar
is still allowed, and is immediately expanded to a dictionary wherefile
is the key for the sidebar file.and automatically expanding to a full dictionary equivalent to
If a dictionary not containing
file
is given tosidebar
, we default to a file named_quartodoc-sidebar.yml
. This leaves the mechanism for including a sidebar the same: the presence of a non-None value forquartodoc.sidebar
indicates that a sidebar file should be written.This allows users to choose
id
and customizecontents
.id
now defaults todir
but follows the user preference, butcontents
allows further customization using a placeholder value to signal where to put quartodoc's contents, following these rules:contents
, use quartodoc's contents.contents
and no sentinel value, append quartodoc's contents to the end of the contents.{{ contents }}
. This includes nested contents!The example in the tests uses this construction. Note that we recursively look for a string in a list exactly matching
"{{ contents }}"
.