Implement relative includes. #37

egonelbre · 2015-10-17T18:42:40Z

Now this works as with relative imports.

toc.md
chapter1/toc.md
chapter1/A.md

Where toc.md contains:

# Chapter 1
{{chapter1/toc.md}}

and chapter1/toc.md:

## A
{{A.md}}

When specifying {{/xyz.md}} will refer to the folder where main file is located. {{/../xyz.md}} will resolve to {{/xyz.md}}, because it is disallowed to go outside of the root folder.

Fixes #36

Issue #36

Previously: * List ``` go A := 123 ``` Created a code-span and didn't include language information. Treat all code with at least 3 accents as a code-block and try to detect language header.

Issue #36

miekg · 2015-10-18T10:27:21Z

README.md

@@ -209,7 +209,8 @@ implements the following extensions:
    to use 2 backslashes it the end of the line.\\

 *   **Includes**, support including files with `{{filename}}` syntax. This is only
-    done when include is started at the beginning of a line.
+    done when include is started at the beginning of a line. The filename is
+    specified with forward slashes `/`.


I would word this as 'The path separator is for the forward slash /. Even on non-Unix environments'.'

miekg · 2015-10-18T10:42:42Z

I like the PR, but would like to see it be minimized or be put into multiple PR that each fix a different thing. The ParseAt name is fine; think the Linux kernel uses the same naming scheme.

egonelbre · 2015-10-18T11:08:58Z

Anyways, as you need it split/minimized... how do these PR-s sound?

Add FileSystem and ParseAt, without relative including, but runs mmark in the "root file" working directory. (This needs to be first so it can be used to test the implementation using virtualFileSystem)
Add language and "isblock" detection for inline code-blocks. (This is necessary to make code-blocks inside lists work.)
Do the code-inclusion in the first-pass. (This is necessary for tracking the correct cwd)
Implement relative includes. (Finally the feature)
A few PR-s for code-simplifications using linespan iterator. (Noticed a few places where it would make code more readable.)

miekg · 2015-10-18T11:49:57Z

As to your final comment: I get the gist of your PR (sorry missed that before). Just cleaning up this one seems fine.

egonelbre · 2015-10-19T14:07:46Z

I hope I fixed all the issues raised.

miekg · 2015-10-19T21:08:28Z

iterator.go

+type linespan struct{ begin, end int }
+
+// next updates begin and end to point to the next line
+func (sc *linespan) next(content []byte) bool {


Thanks for all changes. I however thing this add something new that should happen in another PR. This (linespan) is only used once (now), and there is is basically to prefix each line with the indent.

miekg · 2015-10-19T21:10:18Z

Nice. One nit about linespan.

miekg · 2015-10-20T07:32:41Z

I'll just merge and clean up myself (if I think it is important enough :) )

Thanks for this PR!

Implement relative includes.

egonelbre · 2015-10-20T08:14:34Z

I was just about to start working on this. Anyways, thanks.

The reason I introduced linespan was that, I initially implemented code indenting without it, got it wrong, fixed it and I still had a small bug in it. Basically, it was very difficult for me to keep the relevant pieces in my head and to be sure that I got it right. By using linespan it worked correctly the second time.

I'm certain that a more general linescanner that handles both \r and \n would a lot of the line scanning code much easier to comprehend. Adding some additional things such as line.startsWith and line.extractIndent could make it even more useful.

miekg · 2015-10-20T08:34:12Z

Totally agree. There is one snag though: upstream blackfriday. I still need
to incorporate upstream patches.

However I think this change would be such a large clean-up that I can live
with making it more difficult to incorporate upstream fixes.
On 20 Oct 2015 09:14, "Egon Elbre" notifications@github.com wrote:

I was just about to start working on this. Anyways, thanks.

The reason I introduced linespan was that I initially implement the code
indenting without it, got it wrong, fixed it and still had a small bug in
it. Basically, it was very difficult for me to keep the relevant pieces in
my head and be sure that I got it right. By using linespan it worked
correctly the second time.

I'm certain that a more general linescanner that handles both \r and \n
would a lot of the line scanning code much easier to comprehend. Adding
some additional things such as line.startsWith and line.extractIndent
could make it even more useful.

—
Reply to this email directly or view it on GitHub
#37 (comment).

miekg · 2015-10-21T06:28:07Z

We/I need to double check how images get included with the syntax ![](fig/bumper-inverse.png) and make sure it happens in the same way.

miekg · 2015-10-21T06:34:20Z

So while converting https://github.com/miekg/learninggo to relative includes: I get this:

mmark -page -head inc/head.html -css inc/learninggo.css learninggo.md > learninggo.html
2015/10/21 07:30:17 mmark: failed: `ex/basics/for.md': open ex/basics/ex/basics/for.md: no such file or directory
2015/10/21 07:30:17 mmark: failed: `ex/basics/average-no-func.md': open ex/basics/ex/basics/average-no-func.md: no such file or directory

Which does not seem WAI....

miekg · 2015-10-21T06:40:15Z

Scratch that, everything is relative the the current file.

miekg · 2015-10-21T06:46:37Z

Yep, figure includes don't follow the same rules, see https://github.com/miekg/learninggo/blame/master/ex/functions/stack.md#L9
this gets included relative to the mmark's CWD

egonelbre · 2015-10-21T06:58:12Z

Scratch that, everything is relative the the current file.

You can still do includes relative to root directory with {{/ex/basics/for.md}}.

We/I need to double check how images get included with the syntax and make sure it happens in the same way.

I completely forgot about images. I would rewrite them to absolute during the first-pass and during the second pass handle everything else.

miekg · 2015-10-21T06:59:46Z

Ok,

There is also this little thing

diff --git a/markdown.go b/markdown.go
index 946abd2..e73d584 100644
--- a/markdown.go
+++ b/markdown.go
@@ -445,7 +445,7 @@ func firstPass(p *parser, input []byte, depth int) *bytes.Buffer {
                        if end < lastFencedCodeBlockEnd { // Do not expand tabs while inside fenced code blocks.
                                out.Write(input[beg:end])
                        } else if p.flags&EXTENSION_INCLUDE != 0 {
-                               if input[beg] == '{' {
+                               if beg+2 < len(input) && input[beg] == '{' && input[beg+1] == '{' {
                                        if beg == 0 || (beg > 0 && input[beg-1] == '\n') {
                                                if j := p.include(&out, input[beg:end], depth); j > 0 {

Which makes the tests fail.

miekg · 2015-10-21T07:01:28Z

And this is also interesting:

~/html/learninggo master+  
% make
mmark -page -head inc/head.html -css inc/learninggo.css learninggo.md > learninggo.html
2015/10/21 08:00:19 mmark: handling `@a' as normal text
2015/10/21 08:00:19 mmark: handling `@a' as normal text
2015/10/21 08:00:19 mmark: handling `@a' as normal text
2015/10/21 08:00:19 mmark: handling `@ps' as normal text

This means the Perl (/o) code included is not properly detected as code - digging in right now.

Regardless: I think relative includes make a lot of sense.

miekg · 2015-10-21T07:08:08Z

And with my non-pushed {{-fix:

~/html/learninggo master+  
% make learninggo.txt 
mmark -xml2 -page learninggo.md > learninggo.xml && xml2rfc learninggo.xml
...
2015/10/21 08:07:04 mmark: syntax not supported: FootnoteRef
2015/10/21 08:07:04 mmark: handling `@a' as normal text
2015/10/21 08:07:04 mmark: handling `@a' as normal text
2015/10/21 08:07:04 mmark: handling `@a' as normal text
2015/10/21 08:07:04 mmark: handling `@ps' as normal text
2015/10/21 08:07:04 mmark: section jump from H1 to H3, id: "answer-26"
2015/10/21 08:07:04 mmark: syntax not supported: Footnotes
Parsing file learninggo.xml
WARNING: Parsing Error: Opening and ending tag mismatch: middle line 121 and section, line 5296, column 11
ERROR: Unable to parse the XML document: learninggo.xml

Prolly related to parsing that CodeInclude

miekg · 2015-10-21T21:03:20Z

I'm sorry, but I've reverted this PR. These changes make my learning.md not compile at all and the include perl example <{{process.pl}} does not show up correctly.

This reverts commit 9df0e1b, reversing changes made to ee781d6.

miekg · 2015-10-21T21:14:01Z

I you were to pick this up again, I think a way forward is doing the relative includes first (without changing any of the other logic) and prepare followup PR to move stuff around. And if not, than I'll give it a whack.

egonelbre · 2015-10-21T21:37:55Z

Sad, but I understand. I guess, I can manage with absolute paths or write a minimal markdown parser for my own needs.

I was debugging ~30min to figure out why the perl part isn't working, but didn't reach any good conclusions. The result that comes out of first pass looked correct, although I might have missed something obvious, since there was a lot of it. I was guessing that maybe the code included contained triple accents and terminated the code block early and misaligned rest of it. Although this should've happened with the regular html version as well.

The main problem is that the code including trick can only work, if the fenced blocks have the language header detection. Alternatively if the includes are done with a single pass. Both require changes to the way other code works. I don't think I have a good enough understanding of this code to make a fix that accounts for all corner cases properly (as I initially suspected).

miekg · 2015-10-23T08:25:51Z

Ok, so I gave this a couple of hours and with the current code (based of blackfriday) this is probable insane todo. It works for markdown file includes (that happen in firstPass) but breaks done for code-include which (for know) happen in secondPass - where you loose the info on which file you are working with. This is also problematic for images and stuff because these are inline elements and as such must be dealt with in secondPass.

Of course this can be worked around and everything can be fixed - but this becomes hacky fast and impairs the possibility of incorporating upstream fixes. So I am (also) going to stop implementing this :)

The filename normalizing (i.e. everything uses / - even on windows) is a worthwhile addition though.

egonelbre added 11 commits October 17, 2015 00:57

Add fileSystem and enforce include paths with /

1fa0d30

Issue #36

Include codeblocks during first pass.

50dfad5

Issue #36

Treat multiline ``` fenced as a code-block.

d0b294a

Previously: * List ``` go A := 123 ``` Created a code-span and didn't include language information. Treat all code with at least 3 accents as a code-block and try to detect language header.

Add callouts to the code.

915f837

Add test for learningo regression.

754cd1a

Simplify crashing code.

41e1117

Fix crashing empty line after fenced block.

381fb9f

Don't allow captions for code-blocks inside lists.

9d05968

Rename "span" -> "line" to be consistent.

80b0838

Implement relative imports.

264a610

Issue #36

Merge branch 'master' into includes

b4f5f94

miekg reviewed Oct 18, 2015
View reviewed changes

egonelbre added 5 commits October 19, 2015 16:50

Use clearer wording in README.

810ac7c

Unexport FileSystem.

0e43158

Revert line after code-block fix.

10793f1

Make abspath and fileSystem.ReadFile take []byte.

f537254

Merge branch 'master' into includes

059264c

miekg reviewed Oct 19, 2015
View reviewed changes

miekg added a commit that referenced this pull request Oct 20, 2015

Merge pull request #37 from egonelbre/includes

9df0e1b

Implement relative includes.

miekg merged commit 9df0e1b into miekg:master Oct 20, 2015

miekg added a commit that referenced this pull request Oct 21, 2015

Revert "Merge pull request #37 from egonelbre/includes"

3079af4

This reverts commit 9df0e1b, reversing changes made to ee781d6.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement relative includes. #37

Implement relative includes. #37

egonelbre commented Oct 17, 2015

miekg Oct 18, 2015

miekg commented Oct 18, 2015

egonelbre commented Oct 18, 2015

miekg commented Oct 18, 2015

egonelbre commented Oct 19, 2015

miekg Oct 19, 2015

miekg commented Oct 19, 2015

miekg commented Oct 20, 2015

egonelbre commented Oct 20, 2015

miekg commented Oct 20, 2015

miekg commented Oct 21, 2015

miekg commented Oct 21, 2015

miekg commented Oct 21, 2015

miekg commented Oct 21, 2015

egonelbre commented Oct 21, 2015

miekg commented Oct 21, 2015

miekg commented Oct 21, 2015

miekg commented Oct 21, 2015

miekg commented Oct 21, 2015

miekg commented Oct 21, 2015

egonelbre commented Oct 21, 2015

miekg commented Oct 23, 2015

Implement relative includes. #37

Implement relative includes. #37

Conversation

egonelbre commented Oct 17, 2015

miekg Oct 18, 2015

Choose a reason for hiding this comment

miekg commented Oct 18, 2015

egonelbre commented Oct 18, 2015

miekg commented Oct 18, 2015

egonelbre commented Oct 19, 2015

miekg Oct 19, 2015

Choose a reason for hiding this comment

miekg commented Oct 19, 2015

miekg commented Oct 20, 2015

egonelbre commented Oct 20, 2015

miekg commented Oct 20, 2015

miekg commented Oct 21, 2015

miekg commented Oct 21, 2015

miekg commented Oct 21, 2015

miekg commented Oct 21, 2015

egonelbre commented Oct 21, 2015

miekg commented Oct 21, 2015

miekg commented Oct 21, 2015

miekg commented Oct 21, 2015

miekg commented Oct 21, 2015

miekg commented Oct 21, 2015

egonelbre commented Oct 21, 2015

miekg commented Oct 23, 2015