cmacmackin / markdown-include

Provides syntax for Python-Markdown which allows for the inclusion of the contents of other Markdown documents.
GNU General Public License v3.0
104 stars 41 forks source link

Master #14

Closed patcorwin closed 5 years ago

patcorwin commented 6 years ago

Added flags to clean out metadata from .md files and increase the headers of included file.

patcorwin commented 6 years ago

This is literally my first pull request and I'm fumbling my way through using git so please let me know if I need to do something differently. My use-case is for a wiki, where I want to "nest" the included file's header. I also figured that it was fair to accommodate the metadata plugin since it comes with markdown.

cmacmackin commented 6 years ago

I haven't had a chance to take a close look at what you've done yet, but the idea seems like a good one! I'm pretty sure this will solve some issues I've seen people have. Once I've checked over your code I'll let you know if there's anything that needs changing either in the source or how you did the pull request.

On 02/03/18 11:48, patcorwin wrote:

This is literally my first pull request and I'm fumbling my way through using git so please let me know if I need to do something differently. My use-case is for a wiki, where I want to "nest" the included file's header. I also figured that it was fair to accommodate the metadata plugin since it comes with markdown.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/cmacmackin/markdown-include/pull/14#issuecomment-369900827, or mute the thread https://github.com/notifications/unsubscribe-auth/AHxJPdlinA7jqCY_-YBTsbt8q06_u3ywks5taTF6gaJpZM4SZnSs.

-- Chris MacMackin cmacmackin.github.io https://cmacmackin.github.io

marcelstoer commented 6 years ago

This addition is most welcome!

please let me know if I need to do something differently

Maybe document the new options in README.md?

Inheriting heading depth seems like a good idea but I guess there are also cases where you'd want to explicitly set an offset. For comparison, with AsciiDoc you would do that like this: <include-notation>[leveloffset=+1]

marcelstoer commented 6 years ago

@patcorwin thanks for the changes, I like what you do. However, I'm pretty sure that you'll want to define the optional offset for every include rather than globally. WDYT?

marcelstoer commented 6 years ago

I'm still struggling to make sense of the git pull request workflow but from what I've read it looks like everything just updates automatically.

Yep, you can consider the PR page here as a view onto a specific set of commits in your fork of this repository (that's why it updates automagically).

patcorwin commented 6 years ago

Yeah, when I initially did inheriting depth, I thought it might be useful to have it defined in the include but then forgot about it as I implemented it. Since I haven't used many extensions, I don't know if there is a common practice for passing args per invocations so was thinking something like:

{!filename.md|headingOffset=2 inheritHeadingDepth=False}

I think I pipe, or some other symbol, is required so you know the filename part is over, then space delineated options would be pretty easy to parse out. Again, I've done some searching for an existing standard but haven't found one (and might be bad at searching).

If you think it's a good idea, or have some other ideas on the syntax, I'll make the update.

marcelstoer commented 6 years ago

I would keep inheritance as a global setting and only allow offsets for individual includes to tune the outcome. The syntax looks fine to me (gets easier if inheritance is a global-only setting) but let's hear what @cmacmackin thinks of this.

Btw, you might want to edit the title of this PR; "Master" isn't very expressive 😜

cmacmackin commented 5 years ago

Finally gotten around to looking at this. Most of it looks good, but the bit you have to remove meta-data is broken; if you have more than one paragraphs in an include file it will remove the first one. As such an feature is beyond the scope of the general pull request I have removed it for the time being. I've also rewritten the README slightly so that it flows a bit better.