md_browser: Implement Gitiles blocks notation.

md_browser: Implement Gitiles blocks notation.

This patch implements an extention for Python-Markdown which processes
Gitiles' extended blocks notation, such as:

    *** note
    ...
    ***

Manually tested with existing documentation containing this syntax, such
as docs/linux_debugging.md.

R=nodir@chromium.org, dpranke@chromium.org
BUG=none

Committed: https://crrev.com/cf19a4364f0623982d4a58cefbdec6f4cdded86e
Cr-Commit-Position: refs/heads/master@{#355490}

[Yuta Kitamura, 2015-10-20 11:21:38]

Hi nodir,

Do you think it's reasonable to have this functionality in md_browser? If so,
please take a look.

[nodir, 2015-10-20 14:53:39]

nodir@chromium.org changed reviewers:
+ dpranke@chromium.org

[nodir, 2015-10-20 14:53:39]

+Dirk, the owner

https://codereview.chromium.org/1414863003/diff/1/tools/md_browser/gitiles_ex...
File tools/md_browser/gitiles_ext_blocks.py (right):

https://codereview.chromium.org/1414863003/diff/1/tools/md_browser/gitiles_ex...
tools/md_browser/gitiles_ext_blocks.py:16: from markdown.util import etree
there are style inconsistencies in this file, such as blank lines above and that
you import classes instead of modules, but dpranke@ is more appropriate to
review style

https://codereview.chromium.org/1414863003/diff/1/tools/md_browser/gitiles_ex...
tools/md_browser/gitiles_ext_blocks.py:23: RE_START = re.compile(r'^\*\*\*
(note|aside|promo)\n')
there may be trailing space

https://codereview.chromium.org/1414863003/diff/1/tools/md_browser/gitiles_ex...
tools/md_browser/gitiles_ext_blocks.py:24: RE_END =
re.compile(r'\n\*\*\*(?:\n|$)')
here too

[Dirk Pranke, 2015-10-20 22:42:17]

mostly looks fine, just a few comments.

https://codereview.chromium.org/1414863003/diff/1/tools/md_browser/gitiles_ex...
File tools/md_browser/gitiles_ext_blocks.py (right):

https://codereview.chromium.org/1414863003/diff/1/tools/md_browser/gitiles_ex...
tools/md_browser/gitiles_ext_blocks.py:16: from markdown.util import etree
On 2015/10/20 14:53:39, nodir wrote:
> there are style inconsistencies in this file, such as blank lines above and
that
> you import classes instead of modules, but dpranke@ is more appropriate to
> review style

I'm fine w/ importing classes directly.

https://codereview.chromium.org/1414863003/diff/1/tools/md_browser/gitiles_ex...
tools/md_browser/gitiles_ext_blocks.py:20: class
GitilesExtBlockProcessor(BlockProcessor):
Nit: I'd name this _GitilesExtBlockProcessor, since it doesn't need to be a
public class.

https://codereview.chromium.org/1414863003/diff/1/tools/md_browser/gitiles_ex...
tools/md_browser/gitiles_ext_blocks.py:27: self.__last_parents = []
why is this a list? it seems like having nested blocks isn't allowed, so you
could probably make this a single element and just assert that it is empty on
match start and not empty on match end.

Also, as a nit, I don't usually recommend using __var for private variable
members; the name mangling Python does to ensure privacy is not usually needed
and makes things a bit harder to use.

https://codereview.chromium.org/1414863003/diff/1/tools/md_browser/gitiles_ex...
tools/md_browser/gitiles_ext_blocks.py:28: BlockProcessor.__init__(self, *args,
**kwargs)
Nit: I'd probably do super(GitilesExtBlockProcessor, self)(*args, **kwargs)
instead, but it doesn't matter much.

[Yuta Kitamura, 2015-10-21 04:36:49]

Addressed comments. PTAL?

https://codereview.chromium.org/1414863003/diff/1/tools/md_browser/gitiles_ex...
File tools/md_browser/gitiles_ext_blocks.py (right):

https://codereview.chromium.org/1414863003/diff/1/tools/md_browser/gitiles_ex...
tools/md_browser/gitiles_ext_blocks.py:16: from markdown.util import etree
On 2015/10/20 at 22:42:16, Dirk Pranke wrote:
> On 2015/10/20 14:53:39, nodir wrote:
> > there are style inconsistencies in this file, such as blank lines above and
that
> > you import classes instead of modules, but dpranke@ is more appropriate to
> > review style
> 
> I'm fine w/ importing classes directly.

Thanks, I dropped one blank line from the blanks above.

(I researched the number of blank lines after the end of module
documentation, but it wasn't really consistent: 0, 1 or 2 lines.
One blank line seemed majority, so I followed that.)

https://codereview.chromium.org/1414863003/diff/1/tools/md_browser/gitiles_ex...
tools/md_browser/gitiles_ext_blocks.py:20: class
GitilesExtBlockProcessor(BlockProcessor):
On 2015/10/20 at 22:42:16, Dirk Pranke wrote:
> Nit: I'd name this _GitilesExtBlockProcessor, since it doesn't need to be a
public class.

Done.

https://codereview.chromium.org/1414863003/diff/1/tools/md_browser/gitiles_ex...
tools/md_browser/gitiles_ext_blocks.py:23: RE_START = re.compile(r'^\*\*\*
(note|aside|promo)\n')
On 2015/10/20 at 14:53:39, nodir wrote:
> there may be trailing space

Done.

https://codereview.chromium.org/1414863003/diff/1/tools/md_browser/gitiles_ex...
tools/md_browser/gitiles_ext_blocks.py:24: RE_END =
re.compile(r'\n\*\*\*(?:\n|$)')
On 2015/10/20 at 14:53:39, nodir wrote:
> here too

Done.

https://codereview.chromium.org/1414863003/diff/1/tools/md_browser/gitiles_ex...
tools/md_browser/gitiles_ext_blocks.py:27: self.__last_parents = []
My first assumption was that these blocks were nestable, but
I now think nested blocks are not really useful, so I've changed
this list to a variable.

Renamed __last_parents to _last_parent.

Asserting on inconsistency is probably not desirable; if we did so,
broken documentation can abort the local web server. Instead, we
probably should keep parsing even if the results are corrupted.

https://codereview.chromium.org/1414863003/diff/1/tools/md_browser/gitiles_ex...
tools/md_browser/gitiles_ext_blocks.py:28: BlockProcessor.__init__(self, *args,
**kwargs)
On 2015/10/20 at 22:42:16, Dirk Pranke wrote:
> Nit: I'd probably do super(GitilesExtBlockProcessor, self)(*args, **kwargs)
instead, but it doesn't matter much.

I first did it that way but that didn't work, because BlockProcessor is not a
"new style"
class, i.e. it does not inherit from object.

[Yuta Kitamura, 2015-10-21 04:37:07]

Description was changed from

==========
md_browser: Implement Gitiles blocks notation.

This patch implements an extention for Python-Markdown which processes
Gitiles' extended blocks notation, such as:

    *** note
    ...
    ***

Manually tested with existing documentation containing this syntax, such
as docs/linux_debugging.md.

R=nodir@chromium.org
BUG=none
==========

to

==========
md_browser: Implement Gitiles blocks notation.

This patch implements an extention for Python-Markdown which processes
Gitiles' extended blocks notation, such as:

    *** note
    ...
    ***

Manually tested with existing documentation containing this syntax, such
as docs/linux_debugging.md.

R=nodir@chromium.org, dpranke@chromium.org
BUG=none
==========

[Dirk Pranke, 2015-10-21 19:12:51]

lgtm, thanks!

https://codereview.chromium.org/1414863003/diff/1/tools/md_browser/gitiles_ex...
File tools/md_browser/gitiles_ext_blocks.py (right):

https://codereview.chromium.org/1414863003/diff/1/tools/md_browser/gitiles_ex...
tools/md_browser/gitiles_ext_blocks.py:28: BlockProcessor.__init__(self, *args,
**kwargs)
On 2015/10/21 04:36:48, Yuta Kitamura wrote:
> On 2015/10/20 at 22:42:16, Dirk Pranke wrote:
> > Nit: I'd probably do super(GitilesExtBlockProcessor, self)(*args, **kwargs)
> instead, but it doesn't matter much.
> 
> I first did it that way but that didn't work, because BlockProcessor is not a
> "new style"
> class, i.e. it does not inherit from object.

ok.

[Yuta Kitamura, 2015-10-22 04:40:20]

The CQ bit was checked by yutak@chromium.org

[commit-bot: I haz the power, 2015-10-22 04:40:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1414863003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1414863003/20001

[commit-bot: I haz the power, 2015-10-22 05:00:56]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-10-22 05:01:46]

Patchset 2 (id:??) landed as
https://crrev.com/cf19a4364f0623982d4a58cefbdec6f4cdded86e
Cr-Commit-Position: refs/heads/master@{#355490}