improve only directive

[andreacassioli]

I have posted issues regarding the only directive and always get the reply that what is to me an issue 'it is a specification`.

I do understand that only is a bit peculiar, but still it's behavior is very annoying and limiting. Take thi as example: I want to have a caption for a literalinclude that differs depending on the target:

.. only:: latex

   .. literinclude:: filename
      :caption: c1
      :name: link
.. only:: html

   .. literinclude:: filename
      :caption: c2
      :name: link

Clearly, since I want to link to the block, I need the same name. The result is that using :numref: the link will be recognized as duplicate, and therefore not displayed.

The issue is that nodes in only are processed anyway. No matter on the condition. That is very annoying and in a sense misleading. I think that

1- it should be more clearly described in the docs

2- there should be a way to force the only condition to be evaluated before it's content is parsed, in order to remove it from the very beginning.

I know in some cases I can workaround, but it would require fiddling with extensions, which is not very handy and maintainable. Why cannot a proper conditional be introduced?

See also #1717, #1488 and #2143 (comment)

[andreacassioli]

See also #1911

[fladrinar]

I'm new to sphinx and the main reason I decided it would work for our project was the existence of the only directive.

But now I see that it throws lots of errors because of not excluding the contents of false only directives in the first pass. I'm getting many Duplicate substitution definition name: errors for example, when to me having a variable substitution is the point of a directive like only.

Very frustrated at this point and trying to see how to work around this strange limitation.

[andreacassioli]

I do agree!

Funny it is not a very popular issue...

[lehmannro]

It is a popular issue, but there have been no pull requests so far. It's also tricky to implement this without a major overhaul.

only nodes are currently processed once the builder is available because that's where the tags come from (BuildEnvironment.process_only_nodes.) Everything is already parsed at this point, and that's where the issues with this directive are coming from.

I think most people generally frown upon using only too much because it introduces too much logic into your documents which better be in extensions. Your substitutions sound like a perfect fit for an extension.

The good news last: It should be possible rewriting the Only directive and pulling the logic out of the builder; you can access the environment from a directive and should also be able to access the app at this point in time. I have a patch about 80% done (the only missing piece is re-building the section raising logic.)

[andreacassioli]

I think that, despite it is true that in many cases an extension will do, it is somehow elegant and more portable to rely on standard rst directive and roles. The use of conditionals, and therefore only arises in many situations.

Would you share your patch?

[lehmannro]

As I said, 80% done. I'm gonna submit it once it's done, but I cannot make any guarantees right now. (Might be a couple of weeks.)

Also, as I said, if you're facing yourself with a situation where you have to use only — think again and maybe write an extension. Documents are not programs.

[andreacassioli]

Hi,
thank you again for the feed back. I have made a dirty directive that monkeypatch the official only. I am not sure it will cover all cases, but here it is:

class MyOnly(Directive):
    """
    Directive to only include text if the given tag(s) are enabled.
    """
    has_content = True
    required_arguments = 1
    optional_arguments = 0
    final_argument_whitespace = True
    option_spec = {}

    def run(self):
        #evaluate the condition and if false return no nodes!
        env = self.state.document.settings.env
        if not env.app.builder.tags.eval_condition(self.arguments[0]) :
            return []

        #here I use a container node
        node = nodes.container()
        node.document = self.state.document
        set_source_info(self, node)

        # Same as util.nested_parse_with_titles but try to handle nested
        # sections which should be raised higher up the doctree.
        surrounding_title_styles = self.state.memo.title_styles
        surrounding_section_level = self.state.memo.section_level
        self.state.memo.title_styles = []
        self.state.memo.section_level = 0

        try:
            self.state.nested_parse(self.content, self.content_offset,
                                    node, match_titles=1)
            title_styles = self.state.memo.title_styles

            if (not surrounding_title_styles or
                not title_styles or
                title_styles[0] not in surrounding_title_styles or
                not self.state.parent):
                # No nested sections so no special handling needed.
                return [node]

            # Calculate the depths of the current and nested sections.
            current_depth = 0
            parent = self.state.parent
            while parent:
                current_depth += 1
                parent = parent.parent
            current_depth -= 2
            title_style = title_styles[0]
            nested_depth = len(surrounding_title_styles)
            if title_style in surrounding_title_styles:
                nested_depth = surrounding_title_styles.index(title_style)
            # Use these depths to determine where the nested sections should
            # be placed in the doctree.
            n_sects_to_raise = current_depth - nested_depth + 1
            parent = self.state.parent
            for i in range(n_sects_to_raise):
                if parent.parent:
                    parent = parent.parent

            parent.append(node)
            return []

        finally:
            self.state.memo.title_styles = surrounding_title_styles
            self.state.memo.section_level = surrounding_section_level

then I defined a new directive to make sure I am not messing with the standard only.

[agamalisa]

andrea, that's a very nice addition, but as I'm new to sphinx, I don't really understand how you hook that into your sphinx. I've read up on how to write a module and how to add directives, but it still doesn't work. How did you do it?

[pfalcon]

@andreacassioli : Thanks for making noise and please don't drop the cause, it should be pronounced well that implementation of only:: in Sphinx is SEVERELY BROKEN. I spent a night yesterday unbreaking it for our usecase (which is getting non-existing (i.e. filtered out) stuff also out of genindex and search index). When doing my work, I tried to preserve compatibility with existing usecases of only:: usage, such as generating different content types (i.e. using different builders) on the same cached doctree. Note that from my PoV, that's rather useless usecase, and I have thought that it would be easier to apply only:: processing eagerly (i.e. in a way most people would intuitively expect it to be). I see that you code above does just that. I'm going to test your solution now against mine, as it offers possibility to be added as a sphinx extension (mine requires patching sphinx core, monkey-patching it via extension would be too cumbersome).

[pfalcon]

Well, now I remember why I went my way: to make it warning-clean, e.g. avoid stuff like:

wipy.rst:: WARNING: document isn't included in any toctree

(wipy is referenced in only:: chunk).

[pfalcon]

Also bunch of:

machine.I2C.rst:52: WARNING: exception while evaluating only directive expression: invalid node, check parsing

[pfalcon]

Well, the latter is a result of my monkey-patching implementation.

[pfalcon]

Ok, so I put up couple of monkey-patches to unbreak Sphinx' to do the right thing, in the shape of "extensions", so they can be easily used by the users: https://github.com/pfalcon/sphinx_sane_only .

Maintainers: please fix the illogical behavior upstream.

[andreacassioli]

Hi @pfalcon , thanks for the extensive replies!

Yes my use case is exactly that of avoiding annoying warning and sometimes errors related to missing files. I also think it is a waist of time compiling code that should not.

I see your implementation is very neat!I didn't think to inherit from the original Only class....my monkey patching was wrong as it did not preserver section depth and lead to a bit of a mess. It was OK for my purposes at that time, but then I got into trouble.

I do hope your implementation will be integrated upstream. Maybe a PR will be accepted and that will fix the issue at higher level with no need of extension.

[tk0miya]

The extension has strong restriction to order of building.
It ignores the conditions if you build continuously with multiple builders (cf. make html latexpdf).
You needs to call make clean before switching to another builder.

[pfalcon]

It ignores the conditions if you build continuously with multiple builders (cf. make html latexpdf). You needs to call make clean before switching to another builder.

Yes, that's noted in README. So, I personally don't think it's suitable for mainline - well, not for merging to the core for sure, maybe it would fit sphinx.ext . As I mentioned above, I initially went a different way to resolve the issue - that was not creating index and search index entries immediately, instead store them in the node properties, then have an extra pass after process_only_nodes() to push accumulated data from live nodes into indexes.

Obvious drawback of such solution is that it works for indexes (my usecase), but won't work with gazillion of other things people may use only:: with. So, seeing @andreacassioli's solution, I switched to it (as I mentioned, I had an idea that applying only:: filtering early in the parsing is better idea, I just wasn't sure how to do it properly, e.g. if builder is already set up to evaluate only:: expressions early in the parsing stages).

[pfalcon]

Ok, so in attempt to make it at least formally mergeable into mainline, I switched names used in a project to not be driven by the frustration with this issue. New repo is https://github.com/pfalcon/sphinx_selective_exclude . It still would be better if this was fixed properly in the core, but if for example nothing changes on that front in next half-year, then maybe including such an extension as a workaround might make sense.

[andreacassioli]

Thanks @pfalcon and @tk0miya .

I think an extension is already a big step forward. The caching issue is critical of course, but I think that a good and sound conditional is important as well.

For my purposes, I do not mind loosing caching, as I actually always rebuild all docs. But I see the point.

My question is: is there any way to mark a directive to be cache unfriendly, pretty much as we do for parallel read/write? That way caching would be disabled if that option is used. Maybe it would be even possible to invalidate the whole file that uses that directive.