Re: [Python-markdown-discuss] Announcing Definition List Extension - call for testing

[John S. <jo...@sz...>]

On Tue, Nov 4, 2008 at 10:56 PM, Waylan Limberg <wa...@gm...> wrote:
> I'm happy to report that I just pushed a Definition List extension [1]
> to the repo. I know this has been one of the most requested
> extensions. After Yuri's refactor of the core into a separate
> MarkdownParser, it was much easier to do.

Woot!  I've been eager to have this as well.

> I haven't written docs for it yet, but I have included tests [2],
> which show how it works. See PHP Markdown Extra's docs [3] for the
> hows and whys. Other than the included tests, I haven't really tested
> it much, so please run it through the paces. I'd like to say we have
> everything PHP Markdown Extra has when we release 2.0 and this was the
> biggest hurdle. Now we just need to wrap up the edge cases I'm sure I
> missed.

That would be awesome.

> Any bug reports, patches, test cases, comments, and criticisms are welcome.
>
> [1]: http://gitorious.org/projects/python-markdown/repos/mainline/blobs/master/markdown_extensions%2Fdef_list.py
> [2]: http://gitorious.org/projects/python-markdown/repos/mainline/trees/master/tests/extensions-x-def_list
> [3]: http://michelf.com/projects/php-markdown/extra/#def-list

I haven't tried testing it yet, but I'll definitely be trying it out
soon.  But I do have a couple other comments.

The main markdown parser marked some methods with __'s... which is
supposed to be the indicator that the method is private.  So it's a
little bit disturbing that DefListParser is drilling into those
internals.  Perhaps it's indicative that those methods should be
exposed more, or that we need an extra hook point into the code?

It looks like in _MarkdownParser__processParagraph() that
self.__processHR() and self.__processHeader() need to be
self._MarkdownParser__processHR() and
self._MarkdownParser__processHeader().  By default, python will try
calling self._DefListParser__processHR() and
self._DefListParser__processHeader(), which don't exist.  It looks
like the only difference in your process paragraph is that it also
watches for 'dd'.  Is there some way to either expose that, or just
add this edge to the actual MarkdownParser?  I realize that markdown
doesn't natively produce those tags.  But it seems like a shame to
have to edit the same code in two places if you find a bug (if you
remember the other one exists!).

> ps: I believe the only other things we're missing from extra are
> abbreviations and markdown inside html. If I'm wrong, let me know and
> I'll add it to my todo list. I have an old abbreviations extension
> which needs to be refactored to run with the many recent changes to
> markdown. I haven't even looked at markdown inside html yet.

I think it'd be nice for the table extension to function like PHP
markdown's implementation.  But that's just a nicety. :-)

-John