Question: Approach to be taken for a pull

[facelessuser]

@waylan I had a question about an approach I wanted to run by you, and your general opinion on which direction you'd like me to take for a pull.

Recently you accepted a pull to fix table backticks. In that pull, I hadn't realized that Python Markdown doesn't currently allow for the escape of | in Tables, but the logic I added accounts for it. PHP Markdown Extra does allow this (which makes sense based on the sytax Tables adds). So in general, I would call this a bug, but before I continue, I want to make sure that this is something you want to address, or if you consider this a feature/enhancement that you would prefer not to introduce into the 2.6 series. In that case, I would just issue a pull to remove the logic that accounts for \| and be done.

If you do consider the handling of | to be a bug. The fix is pretty easy, but I wanted to ask about the approach.

1. I could just inject an escape pattern for | and call it done.

2. We could append | to the current escape list and let the current escape pattern handle it. The reason why I like this is that an extension can just see what's already in that list and avoid adding redundant escapes that have already been added by a previous extension. The only issue with this is that a global list is used in the Markdown class. It would be nice if we copied the global array for that instance of Markdown instead so that you could append and not modify the global. I think regardless of whether it is a condoned approach to append to that list, it should probably not be a global copy to protect against people's monkey patching. At the very least make it a tuple so people would have to physically construct a new one if they tried to modify it.

In short:

1. Do you want to handle escaped | for Extra Tables?

2. If so, which approach would you prefer?

Based on your response, I will take the appropriate action.

[waylan]

My intention has always been to have extensions add to the global escape list on the Markdown instance. My thinking has been that if an extension introduces a new character with special meaning, then users should be able to escape that character in the same manner as any other escapable character. And the way to do that is in the global escape list. Yes, that means that you can also escape the character outside of a table, but that is the way escape works for the existing characters anyway. I consider this to be the least surprising behavior for end users.

As an aside, I realize that many of the existing extensions have not done this. That has been an annoyance of mine and part of why I moved the escape list to the class. I just never went through the extensions and made sure they all make use of it.

I should note that the one exception would be when the escaping is context dependent. For example, if a specific character should only be escapable within a table, then implement it specifically in the table extension (this is why the table extension has its own special handling of backticks). But if the escaping is not context dependent (or at least it doesn't need to be), then use the global escape list.

So yes, let's make the | escapable and let's do it with the global escape list.

[waylan]

One additional note: As the global escape list is used as an inlinepattern, there is the possibility that blockprocessor could parse an escaped character as if it was not escaped due to the order things are executed. Although this is not always relevant. For example, an escaped unordered list would simply not parse as a list (the backslash would fail to match the regex). But for a table, that may not be the case as the | doesn't need to be the first non-whitespace character on a line. So we may still need to check that any |s not be preceded by a backslash.

[facelessuser]

I'd like to clarify what I mean by global. I actually use the escape list for a number of my personal extensions. What I found was that if I added to the list in a Markdown instance. If I created another instance and did not include that extension, it would still have that escape.

So what I am actually suggesting is this so that each instance has a list and the base class doesn't get it's copy changed causing all instances moving forward getting modified.

class Markdown(object):
    def __init__(self, *args, **kwargs):  
        self.ESCAPED_CHARS = ['\\', '`', '*', '_', '{', '}', '[', ']', '(', ')', '>', '#', '+', '-', '.', '!']

This is the current behavior:

>>> md = markdown.Markdown()
>>> print(md.ESCAPED_CHARS)
['\\', '`', '*', '_', '{', '}', '[', ']', '(', ')', '>', '#', '+', '-', '.', '!']
>>> md.ESCAPED_CHARS.append('|')
>>> print(md.ESCAPED_CHARS)
['\\', '`', '*', '_', '{', '}', '[', ']', '(', ')', '>', '#', '+', '-', '.', '!', '|']
>>> md2 = markdown.Markdown()
>>> print(md2.ESCAPED_CHARS)
['\\', '`', '*', '_', '{', '}', '[', ']', '(', ')', '>', '#', '+', '-', '.', '!', '|']
>>> 

When I say global , I mean GLOBAL :).

[facelessuser]

I always assumed I was doing something frowned on, so when I noticed this, I started copying the list and modifying the copy and assigned it back.

def escape_chars(md, echrs):
    """
    Add chars to the escape list.
    Don't just append as it modifies the global list permanently.
    Make a copy and extend **that** copy so that only this Markdown
    instance gets modified.
    """

    escaped = copy.copy(md.ESCAPED_CHARS)
    for ec in echrs:
        if ec not in escaped:
            escaped.append(ec)
    md.ESCAPED_CHARS = escaped

[waylan]

What I found was that if I added to the list in a Markdown instance. If I created another instance and did not include that extension, it would still have that escape.

That would be a bug. I never intended it to work that way. It should only ever be global per instance, nothing more.

So what I am actually suggesting is this so that each instance has a list and the base class doesn't get it's copy changed causing all instances moving forward getting modified.

class Markdown(object):
    def __init__(self, *args, **kwargs):  
        self.ESCAPED_CHARS = ['\\', '`', '*', '_', '{', '}', '[', ']', '(', ')', '>', '#', '+', '-', '.', '!']

Yes, that is the way it should be. Please do make that change.

[facelessuser]

Great. I will take care of this. Seems like we are on the same page.