-
Notifications
You must be signed in to change notification settings - Fork 200
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
BUG: templates_path in config was overriding value set in extension #566
BUG: templates_path in config was overriding value set in extension #566
Conversation
thanks for working on this @AakashGfude. Ping me once the tests are passing and I'll review this for you.
|
@mmcky the tests are passing now. Have also updated the link to the sphinx-theme-builder filesystem layout. |
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 @AakashGfude for fixing this bug. I had come across once as well.
@AakashGfude do you think we need a test case for this? |
@mmcky I think that's a good idea. I will create or either modify an existing test case, to test this. |
src/sphinx_book_theme/__init__.py
Outdated
def update_general_config(app): | ||
theme_dir = get_html_theme_path() | ||
# Update templates for sidebar | ||
app.env.config.templates_path.append(os.path.join(theme_dir, "components")) |
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 a comment explaining why this is needed given that we already add the components at the link below?
Otherwise it will confuse people why we do this twice and not just once...
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 thing @choldgraf, I will add the explanation.
@mmcky and @choldgraf this issue is apparently only with jupyter-book builds. As jupyter-book calls https://github.com/executablebooks/jupyter-book/blob/master/jupyter_book/sphinx.py#L7 , which is not in the case of SphinxTestApp. Also, the error is not getting triggered with But, I had to add |
@AakashGfude a good reason to write a test. That is curious.
Thanks for the update. Could it be that the Note: this is useful when running a project without using |
Sphinx overrides any previous config value based on In jupyter-book build the So, is it right to have a jb project here for testing @choldgraf @mmcky , as it might be necessary in some future issues as well? But also we can just not test this particular thing explicitly and save having an extra tests folder and 'jupyter-book' in testing dependencies. |
@AakashGfude so it doesn't seem clear to me if the fix to this templates path issue should be contained in this repository or the I just don't want to introduce circular testing dependences as it can make releases really tricky if this depends on |
@mmcky you are right. I think we should reconsider how we are reading config in For instance, |
@mmcky I can think of only two ways we can solve this in jupyter-book. Either remove |
@AakashGfude where are we on this one? Are you blocked on anything? Do we need to resolve questions before moving forward? |
@choldgraf This PR itself is complete I think, except for the questionable move to test this (which cannot be done without introducing jb here). And we reached a consensus that this is not the right place to fix this.. And had suggested a fix in jupyter-book .
|
I will just create a PR in |
Actually @choldgraf, there is no easy way in |
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 looks good to me - do you know what circumstances "config-inited" is not called? I thought it was always triggered but I guess not. Either way let's merge this in to fix this bug!
- Change `update_general_config()` to run on `config-inited` event in `setup()` method - Add `config` parameter back to `update_general_config()` method
- Change `update_general_config()` to run on `config-inited` event in `setup()` method - Add `config` parameter back to `update_general_config()` method
We update the
templates_path
config variable here https://github.com/executablebooks/sphinx-book-theme/blob/master/src/sphinx_book_theme/__init__.py#L198, to allow sidebars to be found in Jinja templates.But if we set a value to
templates_path
in_config.yml
, then sphinx overrides any previous value set to it, as can be seen from this function https://github.com/sphinx-doc/sphinx/blob/5.x/sphinx/application.py#L246. Resulting in issues like jupyter-book/jupyter-book#1660.This PR does the updation of the
templates_path
inconfig-inited
event instead, which is emitted after the sphinxconfig.init_values()
function.fixes jupyter-book/jupyter-book#1660