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

Add an include directive that allows one filetype to be included in another #7739

Closed
wants to merge 3 commits into from

Conversation

eric-wieser
Copy link
Contributor

@eric-wieser eric-wieser commented May 28, 2020

Feature or Bugfix

  • Feature

Purpose

  • Primarily to allow markdown files to be included within rst files (like m2r's .. mdinclude
  • Works for all other input parsers too

Upstreaming from pygae/galgebra#413, where this was very useful

@@ -383,6 +472,7 @@ def setup(app: "Sphinx") -> Dict[str, Any]:
directives.register_directive('hlist', HList)
directives.register_directive('only', Only)
directives.register_directive('include', Include)
directives.register_directive('includedoc', IncludeDocument)
Copy link
Contributor Author

@eric-wieser eric-wieser May 28, 2020

Choose a reason for hiding this comment

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

This name is bad, would appreciate feedback on a better name

Copy link

@rpanderson rpanderson Jun 9, 2020

Choose a reason for hiding this comment

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

Alternative directives that better distinguish it from include might be: parse, includeother, includeparsed, etc.


# copied code ends
app = self.env.app
filetype = get_filetype(app.config.source_suffix, path)
Copy link
Contributor Author

@eric-wieser eric-wieser May 28, 2020

Choose a reason for hiding this comment

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

Probably makes sense to provide an option to overload this, for files with weird extensions like .inc or .md.inc etc, something like

        filetype = self.options.get('filetype', None)
        if filetype is None:
            filetype = get_filetype(app.config.source_suffix, path))

Copy link
Member

Choose a reason for hiding this comment

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

+0: But nobody does not know about filetype. So we have to add it to our document.

@tk0miya
Copy link
Member

tk0miya commented May 30, 2020

-1: I don't know why such feature is needed. We already have the toctree feature to build multiple documents into the one project. In addition, merging other document is much complicated for both developers and users (path, warning, and so on).

@tk0miya tk0miya added markup type:proposal a feature suggestion labels May 30, 2020
.. rst:directive:option:: start-line
end-line
start-after
end-after
Copy link
Member

Choose a reason for hiding this comment

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

Are these filtering options needed? In my experience of literalinclude directive, many filter options are requested. So I hesitate to add filtering options.

Copy link
Contributor Author

@eric-wieser eric-wieser May 30, 2020

Choose a reason for hiding this comment

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

Yes, see pygae/galgebra#413 for a useful application. It allows me to split a README.md into pieces, and include each one in a different rst document.

These filters are part of the built-in .. include, so it seems reasonable to keep them anyway.

path = relative_path(None, path)
path = nodes.reprunicode(path)
encoding = self.options.get(
'encoding', self.state.document.settings.input_encoding)
Copy link
Member

Choose a reason for hiding this comment

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

config.source_encoding is better for the default value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code is copied from the built-in ..include

encoding=encoding,
error_handler=e_handler)
except UnicodeEncodeError:
raise self.severe(u'Problems with "%s" directive path:\n'
Copy link
Member

Choose a reason for hiding this comment

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

u prefix is no longer needed, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed - it's here because this code is copied directly from docutils


# copied code ends
app = self.env.app
filetype = get_filetype(app.config.source_suffix, path)
Copy link
Member

Choose a reason for hiding this comment

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

+0: But nobody does not know about filetype. So we have to add it to our document.

parser = app.registry.create_source_parser(app, filetype)

sub_document = new_document(path, self.state.document.settings)
parser.parse(rawtext, sub_document)
Copy link
Member

Choose a reason for hiding this comment

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

Why do you generate new document here? It prevents to share local reference context (ex. footnotes, hyperref, etc.).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there another way to invoke the parser?

I figured it didn't matter, because if the included document is in a different file it's very unlikely to reuse footnotes.

Copy link
Member

Choose a reason for hiding this comment

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

I figured it didn't matter, because if the included document is in a different file it's very unlikely to reuse footnotes.

The document object is large database for the doctree. So all references would be broken. Is it okay to conflict node_IDs between parent and included document? I guess :ref: role for included document may not work also.

Is there another way to invoke the parser?

How about parser.parse(rawtext, self.state.document) instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does docutils allow a document to be parsed into multiple times? How does it know where the current insertion point is?

sphinx/directives/other.py Outdated Show resolved Hide resolved
…nother

Most usefully, this allows embedding markdown within rst.
@RabidCicada
Copy link

Holy cow @eric-wieser . Thankyou for your work on this.

I currently have a code base that is heavily locally documented in each subdirectory with README.md's about each component authored by developers that are much more comfortable with Markdown.

I have crafted my system to parse rst, md, plantuml, and auto harvest C++ API docs using doxygen. After following #701 and looking for a good solution...I've settled on replicating my entire folder structure under docs and using softlinks so that relative linking works both natively on gitlab/github with the Markdown documents but also when built by sphinx.

While this does not solve that problem I appreciate your work in contributing something that would be very useful.

@RabidCicada
Copy link

RabidCicada commented Aug 7, 2020

This might partially solve relative linking instead of relying on softlinks. So then it would work on Windows. Any thoughts about if this could take the place of softlinks. I think your .. includeDoc:: in README.rst's with recommonmark could replace my softlink approach.

docs/
    src/
        folder1/
            README.md <softlink>
        folder2/
            README.md <softlink>
        folder3/
            folder4/
                README.md <softlink>
        README.md <softlink>
        index.rst
        conf.py
    build/
    Makefile
folder1/
    README.md
folder2/
    README.md
folder3/
    folder4/
        README.md
README.md

@RabidCicada
Copy link

I also have looked at and followed a lot of the disussion here: #7000. similar issues.

I like your solution here as one that might actually be mergable since while @tk0miya has suggested he supports the idea all attempts so far have been met with only negative criticism of the solution and resistance to changing deep parts of sphinx ( #2354 and #7913). Not that the criticism isn't warranted or that the guardianship of the code base isn't a good thing.

@tk0miya is there a chance that you can:

  • suggest a positive path forward for implementing a solution that will be acceptable to you for this MR?

abravalheri added a commit to pyscaffold/pyscaffoldext-markdown that referenced this pull request Jan 6, 2021
The approach used by recommonmark's own documentation is to include a
symbolic link file inside the `docs` directory, instead of trying to do
a `include`.

References:
readthedocs/recommonmark#191
sphinx-doc/sphinx#701
sphinx-doc/sphinx#7739
abravalheri added a commit to pyscaffold/pyscaffoldext-markdown that referenced this pull request Jan 6, 2021
The approach used by recommonmark's own documentation is to include a
symbolic link file inside the `docs` directory, instead of trying to do
a `include`.

References:
readthedocs/recommonmark#191
sphinx-doc/sphinx#701
sphinx-doc/sphinx#7739
@tk0miya tk0miya closed this May 9, 2021
@tk0miya tk0miya deleted the branch sphinx-doc:3.x May 9, 2021 03:24
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
markup type:proposal a feature suggestion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants