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

Extending Sphinx SingleHtmlFileBuilder and concrete type checks #11640

Closed
kreuzberger opened this issue Aug 24, 2023 · 6 comments
Closed

Extending Sphinx SingleHtmlFileBuilder and concrete type checks #11640

kreuzberger opened this issue Aug 24, 2023 · 6 comments

Comments

@kreuzberger
Copy link

kreuzberger commented Aug 24, 2023

Describe the bug

In sphinx/writers/html5.py in method get_secnumber (and others) there are concrete checks for a builders name.
This breaks builders inheritance and extension.

E.g. if i extend the Singlehtml Builder and add a new name to it (to identify it properly) the code fails to add section numbers to my generated html files.

    def get_secnumber(self, node: Element) -> tuple[int, ...] | None:
        if node.get('secnumber'):
            return node['secnumber']

        if isinstance(node.parent, nodes.section):
==>         if self.builder.name == 'singlehtml':
                docname = self.docnames[-1]
        return None

Therefore there should be a check if the current builder is an inheritance/extension of the singlehtml builder.

Maybe

        if isinstance(self.builder, SingleFileHTMLBuilder):

but i do not know if this breaks some kind of architectural hierarchies or leads to (maybe) unwanted dependencies between builders and the writers.

This seems to affect only this file.

How to Reproduce

useblocks/sphinx-simplepdf#27

Environment Information

Sphinx 6.2.1
Latest

Sphinx extensions

sphinx-simplepdf

Additional context

No response

@picnixz
Copy link
Member

picnixz commented Aug 25, 2023

Builders for the same task (e.g., building HTML or single HTML pages) are not meant to have different names. If you change the singlehtml builder, then Sphinx assumes that your new builder is the only builder for that task.

In general, there should be a single builder when running Sphinx and this builder is uniquely identifier by its name. You shouldn't "switch" between builders (I don't know why you want to identify your builder using a different name also).

@kreuzberger
Copy link
Author

kreuzberger commented Aug 25, 2023

Do not understand the last comment. The builder is an extension of the SingleHtmlFileBuilder, and it is the only builder used for the task.

Like the SingleHtmlFileBuilder extends StandaloneHTMLBuilder, the simplepdf builder extends the SingleHtmlFileBuilder.

Each builder has its own name, the StandaloneHtmlBuilder has the name "html" and the SingleHtmlFileBuilder is "singlehtml".

Class inheritance:

StandaloneHTMLBuilder <-- SingleHtmlFileBuilder <-- SimplePDFBuilder

The writer should identify the instance of the builder by its class type instead of an concrete object name.

If i extend a builder i would expect to give him a unique name.

@picnixz
Copy link
Member

picnixz commented Aug 25, 2023

There are real dependencies between builders and writers. For instance, it may not be true that what the builder has built is compatible with the implementation of the writer. As such, I would suggest rewriting the writers parts that need to be rewritten.

Without a strict name check, we could implement a SingleHtmlFileBuilder that would still not be compatible with the HTMLTranslator (the writer). Also, it's not only part where builders are checked using their name and not their type.

@kreuzberger
Copy link
Author

As a consequence of this design pattern (thas is not obvious for me from the code), shouldn't be there a SingleHtml5Translator as an extension of the HtmlTranslator? And shouldn't be there a big exception if writer and expected builder do not match?

So i could then extend the SingleHtml5Translator (without changing everything) and use it as writer?

@picnixz
Copy link
Member

picnixz commented Aug 26, 2023

Well, the code is like 10y old and many people went over it. Maybe we should have indeed implemented a singehtmlwriter, but I think here practicality beats purity so we went for that approach.

I personally think that sometimes inheritance should have been better handled, especially in autodoc and docutils, but most of the time, the API that appeared to be public was actually meant to be private most of the time (a good example of this is the recent TocTree adapter that was never thought to be public API but was used as such).

@kreuzberger
Copy link
Author

Ok so i close the issue and try to extend HTML5Translator

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants