helper: split_markdown_front_matter

[Hritik14]

helper for istio and mozilla importers
The following two alternatives are also proposed:

def split_markdown_front_matter(lines: str) -> Tuple[str, str]:
    fmlines = []
    mdlines = []
    splitter = mdlines

    for index, line in enumerate(lines.split("\n")):
        if index == 0 and line.strip().startswith("---"):
            splitter = fmlines
        elif line.strip().startswith("---"):
            splitter = mdlines
        else:
            splitter.append(line)

    return "\n".join(fmlines), "\n".join(mdlines)

and the one present here under MPL license.

Signed-off-by: Hritik Vijay hritikxx8@gmail.com

[sbs2001]

This looks good to me, could you swap this with the thing we have in istio and run tests. That way we'll cover some more testcases and the refactor would also be done.

[sbs2001]

As far as the alternative approaches are concerned, I would stick with something dumb and simple (implementation wise).
The mozilla one, and the one in comments seems the simplest. I'm not sure that the one in the comments would work in case the frontmatter doesn't start from first line. Does that even happen ?

[Hritik14]

swap this with the thing we have in istio and run tests

Done.

The mozilla one, and the one in comments seems the simplest

This PR actually originally offers the same code with a minor improvement.

I'm not sure that the one in the comments would work in case the frontmatter doesn't start from first line. Does that even happen ?

I'm not aware of any instance where front matter doesn't start at the very beginning. I looked at some implementations and it appears that it is rather a strict requirement. (Opening fence must begin on the first line of the markdown string/file ref)
Also, as --- is a valid markdown, I don't think it is allowed for it to mark the beginning of front matter without being at the very top.
I've verified this assumption with github gists and it appears to be true.
Here are the results: https://gist.github.com/Hritik14/7d88ed5d9789c0f5d726746d654590dd (github shows front matter in a nice table)

[sbs2001]

@Hritik14 thanks for the research and the refactor.

Let's use the one you suggested in the comments then. Though the current implementation is concise, it's not super obvious.

[Hritik14]

@sbs2001 I've updated it with the one in comments though I'd really appreciate @pombredanne's insight here as he was really in favor of the partition (d8ea801) approach.

[Hritik14]

@sbs2001 @pombredanne ping

[pombredanne]

Thanks! See some comments inline for your consideration.

[pombredanne]

I understand what you are doing here with your "splitter" variable and this is sleek. But this may not be easy to read and understand at least it was not obvious to me at first.

What could you do to make this easier to read?

1. the first test can be done outside of the loop since if the document starts with --- it must be on the first line which can be safely discarded (the first line).
2. with that done, then the only thing left is that this is frontmatter until the line is --- and at this stage you switch to markdown/body. Or you could partition the remaining joined text on ---?

Or may be at least finding a better name than splitter would help? something like bucket?

Here are some alternative ways for your consideration:

frontmatter = []
markdown = []
lines = text.splitlines(False)
if lines[0] == "---":
    lines = lines[1:]
current_bucket = frontmatter
for line in lines:
    if line == "---":
        current_bucket = markdown
        continue
    current_bucket.append(line)

or this one not using any temp lists:

lines = text.splitlines(False)
# discard the first separator
if lines[0] == "---":
    lines = lines[1:]
text = "\n".join(lines)
frontmatter, _, markdown = text.partition("---\n")

The point I am trying to make is that the important thing is to make the code readable and easy to understand such that 6 months from now you or anyone can get what you meant easily. So naming and being explicit and clear matters a lot IMHO.

[Hritik14]

I took your second implementation and now it looks like

    lines = text.splitlines()
    if lines[0] == "---":
        lines = lines[1:]
        text = "\n".join(lines)
        frontmatter, _, markdown = text.partition("\n---\n")
        return frontmatter, markdown

    return "", text

Let me know what you think

In case the loop method is favorable, I'll make the necessary amends.

[Hritik14]

Thanks @pombredanne. I've incorporated the latest changes. Do have a look.

[Hritik14]

@pombredanne ping

[pombredanne]

See a minor nit FYI. Otherwise LGTM!