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

[ENH] Add tag to cli for "only" content #1618

Closed
wants to merge 4 commits into from

Conversation

jdanke
Copy link

@jdanke jdanke commented Jan 28, 2022

Simple addition to the cli to surface the sphinx "tag" command line option to build conditional content. Works like using sphinx-build -t TAG <sourcedir> with sphinx>=0.6 (see sphinx documentation of the open directive.

This addresses issue #1290 (mistakenly refers to "open" directive). Possibly also #1425.

Multiple option allows for boolean TAG expression. For example

jupyter-book build <sourcedir> --tag cowboy --tag cowgirl

Is needed to build the following:

    ```{only} cowboy and cowgirl
     foo
    ```

@welcome
Copy link

welcome bot commented Jan 28, 2022

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out EBP's Code of Conduct and our Contributing Guide, as this will greatly help the review process.

Welcome to the EBP community! 🎉

@codecov
Copy link

codecov bot commented Jan 28, 2022

Codecov Report

Merging #1618 (5d4b324) into master (4e90fa1) will increase coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head 5d4b324 differs from pull request most recent head e3fc2e6. Consider uploading reports for the commit e3fc2e6 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1618      +/-   ##
==========================================
+ Coverage   91.30%   91.32%   +0.02%     
==========================================
  Files           7        7              
  Lines         690      692       +2     
==========================================
+ Hits          630      632       +2     
  Misses         60       60              
Flag Coverage Δ
pytests 91.32% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
jupyter_book/cli/main.py 95.22% <100.00%> (+0.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4e90fa1...e3fc2e6. Read the comment docs.

@choldgraf
Copy link
Member

Thanks for this contribution - I agree that this would be valuable and in general this looks good with the tests+docs!

My one question is whether we want to special-case sphinx configuration like this, or instead make it possible for somebody to provide their own custom configuration to the sphinx_build command for power users who understand Sphinx well enough to make use of it. That might have potentially resolved your problem without needing to change Jupyter Book itself. On the other hand, maybe the tag approach is generally useful enough that others would benefit from making this explicitly supported / documented. What do others think?

@jdanke
Copy link
Author

jdanke commented Feb 3, 2022

Thanks for the comment. Forgive me for the following length. Being specific to get good guidance without trial & error PRs.

TL/DR: what if config.yaml handles the specification of tags, and I post-process the Sphinx object (app) in sphinx_build to manipulate tags, similar to how latex_documents are configured?

NOTE updated to discuss changes to functions inside of config.py module. Clearer as one comment.

Totally agree with the question of whether adding tag to the cli fits the design. The advantage is minimal change to the Jupyter Book code. That may not be a sufficient reason. I get it. Also, it's a partial solution that only adds tags.

If Jupyter Book thinks it's useful to surface "tag manipulation" to the user, another approach is to work through _config.yml to keep it easy. But as far as I can see, that does require changes to Jupyter Book.

The context is that the tags are set, added, and removed during initialization of a Sphinx object. Sphinx achieves this by executing conf.py to call the Tags.add() and Tags.remove() methods. That's why conf.py contains lines like tags.add('cowboy') and tag entries in the confoverrides dict passed to build_sphinx result in warnings when building (e.g., WARNING: unknown config value 'tags_add' in override, ignoring).

Start with something easy: converting JB's _config.yml to sphinx's conf.py.

A user could add/remove tags in _config.yml like

  config                    :   # key-value pairs to directly over-ride the Sphinx configuration
    html_theme              : sphinx_book_theme
    tags_add                : cowboy
    tags_remove             : cowgirl

If those keys are kept in the dict returned by get_final_config, to create a sphinx conf.py, cli.main.sphinx()#L476 is modified like below.

    # sphinx syntax is different for tags
    tags_add = sphinx_config.pop("tags_add", None)
    tags_remove = sphinx_config.pop("tags_remove", None)

    for key in sorted(sphinx_config):
        lines.append(f"{key} = {sphinx_config[key]!r}")

    if tags_add:
        if not isinstance(tags_add, list):
            tags_add = [tags_add]
        
        for tag in tags_add:
            lines.append(f"tags.add('{tag}')")

    if tags_remove:
        if not isinstance(tags_remove, list):
            tags_remove = [tags_remove]
        
        for tag in tags_remove:
            lines.append(f"tags.remove('{tag}')")  

That smells a little like a workaround.

What I suggest is that the proposed keys tags_add and tags_remove are removed from the dict inside config.yaml_to_sphinx() and returned in another dict. Then config.get_final_config() also has to return the dict. That way sphinx_config is "as expected", cli.main.sphinx() uses the new dict to append the tags.add() and tags.remove() lines when writing conf.py, and sphinx.build_sphinx() uses the new dict to tweak the app.tags.tags :

(just noting, with the new conf.py and its location confdir building with sphinx in sphinx.build_sphinx() (below) would handle adding and removing tags, but is prevented because of patch_docutils. I don't know why that is there, so it's probably not wise to tinker with it.

with patch_docutils(confdir), docutils_namespace():
        app = Sphinx(
            srcdir=sourcedir,
            confdir=confdir,
            outdir=outputdir,
            doctreedir=doctreedir,
            buildername=builder,
            confoverrides=sphinx_config,

I suggest doing a similar trick as above: popping the tag-related tags from sphinx_config prior to instantiation to avoid the warning, then adding code to add/remove tags to app.tags.tags before calling app.build(force_all, filenames), just like how default_latex_documents is handled.

        # add or remove tags here because Sphinx executes conf.py
        # but conf.py not available to when initializing app

        # these would be popped inside of `yaml_to_sphinx()` and pass through `get_final_config()`
        tags_add = sphinx_config.pop("tags_add", None)
        tags_remove = sphinx_config.pop("tags_remove", None)

        if tags_add:
            if not isinstance(tags_add, list):
                tags_add = [tags_add]

            for tag in tags_add:
                app.tags.tags[tag] = True

         if tags_remove:
            if not isinstance(tags_remove, list):
                tags_remove = [tags_remove]

            for tag in tags_remove:
                app.tags.tags.pop(tag, None)

@jdanke
Copy link
Author

jdanke commented Feb 4, 2022

Okay, I'll submit a different PR that work with the configuration yaml instead of the command line.

Do you close this or do I?

@choldgraf
Copy link
Member

@jdanke - sounds good, I'll close this one and take a look at #1630

I must admit it is a bit tough to wrap my head around all of this but I'll give it a shot haha, I've never used this tags functionality in Sphinx before!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants