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

Markdown fragment files #403

Open
WattsC-90 opened this issue Mar 1, 2020 · 11 comments
Open

Markdown fragment files #403

WattsC-90 opened this issue Mar 1, 2020 · 11 comments

Comments

@WattsC-90
Copy link

Is it possible, with the library as it is, to suport fragmented markdown files? I am looking to hse this library as a documentation tool, and the only feature i cant see listed is file fragments! If not, ill have a dig through the code when i get a moment..

@MihaZupan
Copy link
Collaborator

Can you show a small example of what you mean? Like C#'s partial classes, but for markdown?

@WattsC-90
Copy link
Author

Kind of, but with the ability for the partial file to be unpackaged at the inclusion line..
See this plugin for vs code https://marketplace.visualstudio.com/items?itemName=yzane.markdown-pdf in the section markdown-it-include. Unfortunately i cant permalink

@MihaZupan
Copy link
Collaborator

There is no out-of-the-box support for something like this, but it should be rather straight forward to implement as an extension.

For example, you can implement a parser to catch the include syntax and emit a MarkdownIncludeBlock.
Then implement an HtmlRenderer<MarkdownIncludeBlock> to emit the Markdown.ToHtml of the specified file.

@WattsC-90
Copy link
Author

WattsC-90 commented Mar 4, 2020

@MihaZupan I have a preliminary cut of the code working, I have found some restrictions though with potential rendering implementation.. I dont want to ruin the commit history though.. is it OK to commit and push on a branch to the repo?

@MihaZupan
Copy link
Collaborator

If you have a local change, you can fork this repo and push your changes there.

WattsC-90 added a commit to WattsC-90/markdig that referenced this issue Mar 5, 2020
The root.md file "includes" another "includeTest.md"
xoofx#403
WattsC-90 added a commit to WattsC-90/markdig that referenced this issue Mar 5, 2020
Added the parser to the pipeline builder
Created the html renderer which basically recurses and added it to the HtmlRenderer class.

xoofx#403
@WattsC-90
Copy link
Author

WattsC-90 commented Mar 5, 2020

done =] welcome the feedback (but will add a disclaimer.. it was late and i was tired) not that a coder has used THAT excuse before ;)

The SimpleExample project has two markdown files and shows the include line working/rendering

@MihaZupan
Copy link
Collaborator

it was late and i was tired

That's when the cool code appears.

Thanks. Yes, that would be the general idea for approaching it.
From the comments in the code, I see you've also thought about a few problems I'll mention below.

This could be a neat extension, but it would require a few more usability additions, at minimum:

  1. Specifying the root directory for include files (a parameter when adding calling UseMarkdownInclude or somthing similar), or alternatively a callback that fetches the filepath/source based on the path (just a Func<string, string>).
  2. Recursion detection (with throw/ignore behavior) - this one might be more tricky since we don't get the source file name, but we can still detect it later, eg. main.md > header.md > main.md > header.md (boom), as we see the included file name.

I would be careful about using :[alternate-text](includeTest.md) as the syntax as it could be easy to produce false-positives with links to other files. What the extension you linked to does is !!!include(header.md)!!!, but that's up to personal preference and could be made configurable if needed.

Comments about the implementation:

  • Instead of adding the parser/renderer manually, you can make an extension. The extension then adds the parser and renderer to the pipeline, but you get the benefit of easily flowing configuration.
    For example you have MarkdownPipelineBuilder.UseMarkdownInclude(SomeConfig) which adds a new MarkdownIncludeExtension(SomeConfig) to the pipeline. The extension then adds the parser and renderer while flowing the configuration to them. It can also capture the pipeline, so that you can use it in the renderer.
  • In the parser, we can first parse just the indexes/lengths and only allocate substrings when we know we hit some valid include syntax. Alternatively a simple win is to replace new StringBuilder() with StringBuilderCache.Local(). A side note on StringBuilder: it's more efficient to reuse it by doing sb.Length = 0 than to allocate a new one.

To concider: Optionally cache the included files? I would expect the common use case for this to include the header/footer that doesn't change.

@WattsC-90
Copy link
Author

I would be careful about using :[alternate-text](includeTest.md) as the syntax as it could be easy to produce false-positives with links to other files. What the extension you linked to does is !!!include(header.md)!!!, but that's up to personal preference and could be made configurable if needed.

That's weird.. this confused me so I looked at the page i linked to, the markdown-it-include uses the format you suggest, but the markdown pdf extension for VS Code uses the one I implemented.. I agree the format include(path) is better given the closeness to the link style/format. I will switch it over to use the markdown-it-include format instead, in addition the "alternate name" doesnt actually make sense in this context either so its just plain unnecessary.

The path is assumed to always be local/relative, but a check could be added to enable that not being the case.

A side note on StringBuilder: it's more efficient to reuse it by doing sb.Length = 0 than to allocate a new one.

See what I mean about coding tired! I knew that ! :( I didn't know about the StringBuilderCache though, that will be a definite improvement and reduce the allocations.

So moving forward:

  1. Change the pattern to !!!include(file)!!!
  2. Move to an extension
  3. Handle passing in the pipeline/context
  4. Setting include root path as a configuration (default to .)
  5. [further down the line] recursion/loop detection
  6. [further down the line] caching for footers etc.

Thanks for the feedback!

@xoofx
Copy link
Owner

xoofx commented Mar 5, 2020

I have suggested this in the past, but sometimes this problem is handled differently through a text templating (e.g Jekyll with GitHub pages). You can use something like scriban for example (I did use it along Markdig to implement a prototype of static website generator for instance).

But if a markdown syntax is not enough common (supported by a few of the major Markdown engine), I would prefer not to be included in Markdig default extensions. For example, they are using their own include syntax in DocFx and it is working fine for their case.

@ipoley-r7
Copy link

@WattsC-90 - Do you have your latest changes committed anywhere? I'd love to leverage what you've created so far and extend upon it if need be. Nice work!!!

@WattsC-90
Copy link
Author

Unfortunately not, given everything covid related at the moment iv been working from home and as such tried to stay away from code in the evenings! Wasnt ever leaving the screen otherwise. My intention was to try do a couple of hours this weekend which will move it to an extension as opposed to fully inside the main engine.
I will let you know where I get to..

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

No branches or pull requests

4 participants