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

Allow including text/x-rst outputs in rst conversion, transition away from text/restructuredtext #2167

Merged
merged 7 commits into from
Aug 28, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/workflows/tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,8 @@ jobs:

- name: Run tests
run: |
# Attempt to work around https://github.com/pypa/pip/issues/12781
PIP_CONSTRAINT= hatch env run -e test -- pip install 'pip>=24.2'
xvfb-run --auto-servernum hatch run test:nowarn || xvfb-run --auto-servernum hatch run test:nowarn --lf

test_prereleases:
Expand Down
9 changes: 9 additions & 0 deletions nbconvert/exporters/rst.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
from traitlets import default
from traitlets.config import Config

from ..filters import DataTypeFilter
from .templateexporter import TemplateExporter


Expand All @@ -25,6 +26,14 @@ def _template_name_default(self):
output_mimetype = "text/restructuredtext"
export_from_notebook = "reST"

def default_filters(self):
"""Override filter_data_type to use native rst outputs"""
dtf = DataTypeFilter()
dtf.display_data_priority = [self.output_mimetype, *dtf.display_data_priority]
filters = dict(super().default_filters())
filters["filter_data_type"] = dtf
return filters.items()

@property
def default_config(self):
c = Config(
Expand Down
1 change: 1 addition & 0 deletions nbconvert/exporters/templateexporter.py
Original file line number Diff line number Diff line change
Expand Up @@ -407,6 +407,7 @@ def from_notebook_node( # type:ignore[explicit-override, override]
"""
nb_copy, resources = super().from_notebook_node(nb, resources, **kw)
resources.setdefault("raw_mimetypes", self.raw_mimetypes)
resources.setdefault("output_mimetype", self.output_mimetype)
resources["global_content_filter"] = {
"include_code": not self.exclude_code_cell,
"include_markdown": not self.exclude_markdown,
Expand Down
3 changes: 3 additions & 0 deletions share/templates/base/display_priority.j2
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@
{%- elif type == 'application/vnd.jupyter.widget-view+json' -%}
{%- block data_widget_view -%}
{%- endblock -%}
{%- elif type == resources.output_mimetype -%}
{%- block data_native -%}
{%- endblock -%}
{%- else -%}
{%- block data_other -%}
{%- endblock -%}
Expand Down
4 changes: 4 additions & 0 deletions share/templates/rst/index.rst.j2
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,10 @@
{{ output.text | indent }}
{% endblock stream %}

{% block data_native %}
{{ output.data['text/restructuredtext'] }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it also support text/x-rst and text/prs.fallenstein.rst? text/restructuredtext seems pretty non-standard to me (although I acknowledge that this is what nbconverte is already using for some reason)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, good point. 🤔

I think it's best for users to have 1 MIME type for rst, rather than trying to wrangle several. So maybe we should change RSTExporter.output_mimetype as well - probably to text/x-rst, since that's what docutils suggests.

What would break if we do that?

  • Raw cells in notebooks marked with the mimetype text/restructuredtext would stop showing up - but we could work around that by overriding raw_mimetypes to add back text/restructuredtext for compatibility.
  • There's a mechanism for finding templates based on output_mimetype, so custom templates marked text/restructuredtext would no longer work. I can't imagine that many people have custom rst templates, and it's an easy change, so I'd be inclined to document that as a breaking change rather than adding extra complexity to keep them working.

Does that sound reasonable?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is reasonable. If I were doing the change I would probably try to make it as backward compatible as possible.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to ping me for review once you think it is ready

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've done what I described, with compatibility for raw cells using the old mimetype but not for custom templates.

I'm generally keen to keep backwards compatibility, but my impression is that it's unusual to use custom templates at all, it's an easy fix for anyone affected, and the logic for finding templates is already complex enough, so I'm reluctant to add yet another dimension to it to allow multiple mimetypes per exporter. If you want to insist, though, let me know and I'll try to add compatibility for that too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I trust your judgement. If your assessment is accurate, I would say it might be ok to release and see if anyone complains and only then add back the extra compatibility layer.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

{% endblock data_native %}

{% block data_svg %}
.. image:: {{ output.metadata.filenames['image/svg+xml'] | urlencode }}
{% endblock data_svg %}
Expand Down
75 changes: 75 additions & 0 deletions tests/exporters/files/rst_output.ipynb
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
{
"cells": [
{
"cell_type": "code",
"execution_count": 1,
"id": "d5202f0f-21b8-4509-a9e2-45caf1c7db7a",
"metadata": {},
"outputs": [],
"source": [
"from textwrap import indent\n",
"\n",
"\n",
"class Note:\n",
" def __init__(self, text):\n",
" self.text = text\n",
"\n",
" def _repr_html_(self):\n",
" return f'<div style=\"font-weight: bold; font-size: 16pt;\">{self.text}</div>'\n",
"\n",
" def _repr_mimebundle_(self, include=None, exclude=None):\n",
" return {\"text/restructuredtext\": \".. note::\\n\\n\" + indent(self.text, \" \")}"
]
},
{
"cell_type": "code",
"execution_count": 2,
"id": "5145b05f-3a07-4cff-8738-516a9c27cb58",
"metadata": {},
"outputs": [
{
"data": {
"text/html": [
"<div style=\"font-weight: bold; font-size: 16pt;\">Testing testing</div>"
],
"text/plain": [
"<__main__.Note at 0x7fe804028740>"
],
"text/restructuredtext": [
".. note::\n",
"\n",
" Testing testing"
]
},
"execution_count": 2,
"metadata": {},
"output_type": "execute_result"
}
],
"source": [
"Note(\"Testing testing\")"
]
}
],
"metadata": {
"kernelspec": {
"display_name": "Python 3 (ipykernel)",
"language": "python",
"name": "python3"
},
"language_info": {
"codemirror_mode": {
"name": "ipython",
"version": 3
},
"file_extension": ".py",
"mimetype": "text/x-python",
"name": "python",
"nbconvert_exporter": "python",
"pygments_lexer": "ipython3",
"version": "3.12.4"
}
},
"nbformat": 4,
"nbformat_minor": 5
}
11 changes: 11 additions & 0 deletions tests/exporters/test_rst.py
Original file line number Diff line number Diff line change
Expand Up @@ -68,3 +68,14 @@ def test_png_metadata(self):
assert ":width:" in attr_string
assert ":height:" in attr_string
assert "px" in attr_string

def test_rst_output(self):
"""
Is native text/restructuredtext output included when converting
"""
(output, resources) = RSTExporter().from_filename(
self._get_notebook(nb_name="rst_output.ipynb")
)
assert len(output) > 0
assert "\n.. note::" in output
assert ".. raw:: html" not in output # rst should shadow html output
Loading