-
Notifications
You must be signed in to change notification settings - Fork 568
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
Conversation
The remaining test failure is because a pip bug broke installing pandocfilters 1.4.1. That's already been fixed in pip 24.2, but the environment that hatch creates still seeems to get pip 24.1. 😮💨 |
c1390ca
to
bb09112
Compare
I've got all the CI passing now. |
I don't suppose I can interest anyone in reviewing this? 🙂 |
share/templates/rst/index.rst.j2
Outdated
@@ -44,6 +44,10 @@ | |||
{{ output.text | indent }} | |||
{% endblock stream %} | |||
|
|||
{% block data_native %} | |||
{{ output.data['text/restructuredtext'] }} |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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 overridingraw_mimetypes
to add backtext/restructuredtext
for compatibility. - There's a mechanism for finding templates based on
output_mimetype
, so custom templates markedtext/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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
text/x-rst
outputs in rst conversion
text/x-rst
outputs in rst conversiontext/x-rst
outputs in rst conversion, transition away from text/restructuredtext
I updated the title to make it explicit in the changelog - please adjust if you find better wording :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @takluyver!
Outputs in a notebook can include arbitrary mimetypes. From IPython this can be done with
_repr_mimebundle_
method, for instance. Most of the export formats that nbconvert knows about have a corresponding template block for output already in that format, e.g.data_latex
ordata_html
. However, there's no template block for rst output.I've partially generalised this, by adding a
data_native
block to the display priority template rather than a specificdata_rst
. This is meant to hold output in the exporter's native format (itsoutput_mimetype
), iff there isn't already a dedicated block for that format. I admit that's not the tidiest definition, but it seemed the best way to avoid breaking anything.The exporter class then has to customise
filter_data_type
to accept its additional mimetype. I've done this inRSTExporter
. It could be generalised inTemplateExporter
, but that runs more risk of surprising behaviour for subclasses, which are unlikely to have adata_native
block in their templates.