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

Changing source_suffix in an extension's setup() function? #2162

Closed
mgeier opened this issue Dec 9, 2015 · 10 comments
Closed

Changing source_suffix in an extension's setup() function? #2162

mgeier opened this issue Dec 9, 2015 · 10 comments
Milestone

Comments

@mgeier
Copy link
Contributor

mgeier commented Dec 9, 2015

I'm writing an extension which provides a custom source parser for *.ipynb files.

I'm trying to append '.ipynb' to app.config.source_suffix within my extension's setup() function, to make it easier for my users. This way, they don't have to specify source_suffix explicitly in their conf.py.

This worked perfectly until I tried to actually specify source_suffix in conf.py. As soon as source_suffix is defined there, my appending '.ipynb' ceases to work, and my *.ipynb files are not found raising a warning like this:

.../doc/index.rst:6: WARNING: toctree contains reference to nonexisting document 'example'

If I remove the assignment to source_suffix from conf.py, it works again.

Any idea what's going wrong here?

Is there anything I can do to make this work?

@mgeier
Copy link
Contributor Author

mgeier commented Jan 3, 2016

OK, I had a closer look at https://github.com/sphinx-doc/sphinx/blob/master/sphinx/application.py and found an ugly solution. I still don't understand every detail, so I still hope for a better and less ugly solution!

The problem seems to be that in Sphinx.__init__() the self.config object is created, but the values from conf.py are not immediately evaluated. Instead, they are temporarily stored in self.config._raw_config.
After that, the extensions are loaded and their setup() functions are called.
And after that, the config values are finally parsed with self.config.init_values().

My hackish solution is to manipulate app.config._raw_config instead of the attributes of app.config themselves:

class NotebookParser(...):
    ...

def setup(app):
    source_suffix = app.config._raw_config.get('source_suffix', ['.rst'])
    if isinstance(source_suffix, sphinx.config.string_types):
        source_suffix = [source_suffix]
    if '.ipynb' not in source_suffix:
        source_suffix.append('.ipynb')
        app.config._raw_config['source_suffix'] = source_suffix
    source_parsers = app.config._raw_config.get('source_parsers', {})
    if '.ipynb' not in source_parsers and 'ipynb' not in source_parsers:
        source_parsers['.ipynb'] = NotebookParser
        app.config._raw_config['source_parsers'] = source_parsers

A less ugly solution would be highly appreciated!

tk0miya added a commit to tk0miya/sphinx that referenced this issue Jan 4, 2016
@birkenfeld
Copy link
Member

The issue is that extensions (i.e. their setup() functions) are allowed to add new config values. Therefore, the full initialization of config values is only done after all extensions have been set up.

We could switch to a two-stage model, initializing all built-in values before, and only doing the newly added ones afterwards.

@tk0miya
Copy link
Member

tk0miya commented Jan 4, 2016

I think two stage model is too much solution in this case.
In PR #2162, I merged config values from conf.py and extention after setting up finished.

Now, both source_suffix and source_parsers are used only for enable additional parsers. not for any configurations.
So, I think it should be better that the users never mind these config values and enable parsers through adding extension or not.

@mgeier
Copy link
Contributor Author

mgeier commented Jan 4, 2016

@birkenfeld: Thanks for the explanations, now I get it, this totally makes sense!

I don't know how much effort a two-stage thing would be, but it would be definitely easier to understand for the user (= me).
Then changing attributes of app.config within an extension would be very simple and obvious.
The fact that this isn't supposed to work for extension-defined settings is also quite obvious.

OTOH, I'm also fine with @tk0miya's PR #2209, which works perfectly for my current case.
But who knows, other people (or future me) might want to change other settings from their extensions ...

mgeier added a commit to spatialaudio/nbsphinx that referenced this issue Jan 4, 2016
@mgeier
Copy link
Contributor Author

mgeier commented Jan 5, 2016

FWIW, I found a different use for a more general solution: My extension also changes latex_elements.
For now I'm doing this with the 'builder-inited' event, but doing it in setup() would be much more straightforward.

Not a biggie, though.

@tk0miya
Copy link
Member

tk0miya commented Jan 6, 2016

But who knows, other people (or future me) might want to change other settings from their extensions ...

The source parsers are components for docutils and Sphinx.
At this present, we use config variables (source_suffix and source_parsers) to install them.
But, on the other hand, other components (builders, nodes, directives, roles, domains, tranlators, and so on) are installed by Sphinx API.
So I think it should be introduce Sphinx API to install them.
(In additon, It would be nice if we can deprecate these config variables for parsers in future version.)

Of course, setting up config variables by extentions is important, too. But, these are separated issue, I think.

For now I'm doing this with the 'builder-inited' event, but doing it in setup() would be much more straightforward.

The source_suffix and source_parsers are used in builder initialization.
So the event is late to set up them.

tk0miya added a commit that referenced this issue Jan 17, 2016
Fix #2162: Add Sphinx.add_source_parser() to add source_suffix and source_parsers from extension
tk0miya added a commit that referenced this issue Jan 17, 2016
@tk0miya tk0miya added this to the 1.4 milestone Jan 17, 2016
@tk0miya
Copy link
Member

tk0miya commented Jan 17, 2016

Now, I merged #2209. And then, The extensions are able to add source_parsers using Sphinx.add_source_parser().
Please use the brandnew API :-)

Thanks,

@mgeier
Copy link
Contributor Author

mgeier commented Jan 17, 2016

Thanks a lot!

What about the two-stage model that @birkenfeld brought up?
Shall we create a new issue or was this rejected?

@tk0miya
Copy link
Member

tk0miya commented Jan 17, 2016

@mgeier Oh, I missed it.
Could you create a new issue for the model?

@mgeier
Copy link
Contributor Author

mgeier commented Jan 20, 2016

done: #2255

mgeier added a commit to spatialaudio/nbsphinx that referenced this issue Feb 3, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants