Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2482703002:
Change [TOC] processing in MD browser to match gitiles (Closed)
|
Created:
4 years, 1 month ago by Dirk Pranke Modified:
4 years, 1 month ago Reviewers:
jsbell CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionChange [TOC] processing in MD browser to match gitiles
This change adds a processing pass to md_browser's markdown handling
so that if a document only has a single H1, the [TOC] plugin will
ignore it. This change also adds a "Contents" H2 to better match
Gitiles' output.
R=jsbell@chromium.org
BUG=590280
Committed: https://crrev.com/ded33c74ac7aea5962940660717346b65db4d9d0
Cr-Commit-Position: refs/heads/master@{#432000}
Patch Set 1 #
Total comments: 6
Patch Set 2 : clean up logic, add more comments #Patch Set 3 : reparent #
Total comments: 2
Patch Set 4 : fix nesting of ULs in the non-single-h1 case #
Total comments: 4
Patch Set 5 : update with more review feedback, clean up, rewrite comments #Messages
Total messages: 16 (2 generated)
https://codereview.chromium.org/2482703002/diff/1/tools/md_browser/md_browser.py File tools/md_browser/md_browser.py (right): https://codereview.chromium.org/2482703002/diff/1/tools/md_browser/md_browser... tools/md_browser/md_browser.py:221: # Also, we want to insert an <h2>Contents and then indent Can you depict the "after" structure as you did the "before" structure? https://codereview.chromium.org/2482703002/diff/1/tools/md_browser/md_browser... tools/md_browser/md_browser.py:224: if toc_nodes: Maybe early return if falsy, to reduce indentation? And/or I'd expect a loop over toc_nodes or just find()/processing the first one, but I don't understand the else branch below so I don't know if my expectation is correct. Is class=toc assigned to anything but the top-level TOC node? https://codereview.chromium.org/2482703002/diff/1/tools/md_browser/md_browser... tools/md_browser/md_browser.py:233: children = list(toc) What is toc in this else branch, and below if this branch is taken? (I have a feeling there's an "alternate" TOC structure that isn't illustrated where populating children not from toc_ul_li_ul makes sense, but I don't see toc initialized for this branch)
https://codereview.chromium.org/2482703002/diff/1/tools/md_browser/md_browser.py File tools/md_browser/md_browser.py (right): https://codereview.chromium.org/2482703002/diff/1/tools/md_browser/md_browser... tools/md_browser/md_browser.py:221: # Also, we want to insert an <h2>Contents and then indent On 2016/11/07 17:32:06, jsbell wrote: > Can you depict the "after" structure as you did the "before" structure? Will do. https://codereview.chromium.org/2482703002/diff/1/tools/md_browser/md_browser... tools/md_browser/md_browser.py:224: if toc_nodes: On 2016/11/07 17:32:06, jsbell wrote: > Maybe early return if falsy, to reduce indentation? Good suggestion, will do. > And/or I'd expect a loop over toc_nodes or just find()/processing the first one, > but I don't understand the else branch below so I don't know if my expectation > is correct. Is class=toc assigned to anything but the top-level TOC node? Looks like the logic in lines 225-233 is wrong. The intent was to only flatten the tree if there was a single H1, otherwise to leave it alone, but that's not what it does. Will fix. https://codereview.chromium.org/2482703002/diff/1/tools/md_browser/md_browser... tools/md_browser/md_browser.py:233: children = list(toc) On 2016/11/07 17:32:06, jsbell wrote: > What is toc in this else branch, and below if this branch is taken? > > (I have a feeling there's an "alternate" TOC structure that isn't illustrated > where populating children not from toc_ul_li_ul makes sense, but I don't see toc > initialized for this branch) See above, will fix.
okay, updated. Please take another look?
https://codereview.chromium.org/2482703002/diff/40001/tools/md_browser/md_bro... File tools/md_browser/md_browser.py (right): https://codereview.chromium.org/2482703002/diff/40001/tools/md_browser/md_bro... tools/md_browser/md_browser.py:260: children = list(toc_node) I'm unfamiliar with ElementTree and the exact structure in this case, but assuming the structure is the same (<div><ul><li>...) it looks like since toc_node is the <div>, children here would end up being just the <ul>, whereas we want it to be the list of <li>s?
Updated again. Take 3? https://codereview.chromium.org/2482703002/diff/40001/tools/md_browser/md_bro... File tools/md_browser/md_browser.py (right): https://codereview.chromium.org/2482703002/diff/40001/tools/md_browser/md_bro... tools/md_browser/md_browser.py:260: children = list(toc_node) On 2016/11/08 17:29:12, jsbell wrote: > I'm unfamiliar with ElementTree and the exact structure in this case, but > assuming the structure is the same (<div><ul><li>...) it looks like since > toc_node is the <div>, children here would end up being just the <ul>, whereas > we want it to be the list of <li>s? Ah, good catch.
lgtm with two possible simplifications https://codereview.chromium.org/2482703002/diff/60001/tools/md_browser/md_bro... File tools/md_browser/md_browser.py (right): https://codereview.chromium.org/2482703002/diff/60001/tools/md_browser/md_bro... tools/md_browser/md_browser.py:260: children = list(toc_ul) this could just be children = toc_ul I think https://codereview.chromium.org/2482703002/diff/60001/tools/md_browser/md_bro... tools/md_browser/md_browser.py:261: while len(toc_node): toc_node should only ever have one child (toc_ul). So this loop is unnecessary. It could also be moved out of the if/else block.
Okay, your latest set of comments led me to rework things a bit. Let me know what you think now? https://codereview.chromium.org/2482703002/diff/60001/tools/md_browser/md_bro... File tools/md_browser/md_browser.py (right): https://codereview.chromium.org/2482703002/diff/60001/tools/md_browser/md_bro... tools/md_browser/md_browser.py:260: children = list(toc_ul) On 2016/11/08 22:38:04, jsbell wrote: > this could just be children = toc_ul I think I think you're right. I can't say I like this implicit use of iterators either way, though. https://codereview.chromium.org/2482703002/diff/60001/tools/md_browser/md_bro... tools/md_browser/md_browser.py:261: while len(toc_node): On 2016/11/08 22:38:04, jsbell wrote: > toc_node should only ever have one child (toc_ul). So this loop is unnecessary. > It could also be moved out of the if/else block. Yes, I think you're right.
lgtm
lgtm
On 2016/11/14 23:22:16, jsbell wrote: > lgtm Thanks! And thanks for the detailed comments, the code is better for it.
The CQ bit was checked by dpranke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Change [TOC] processing in MD browser to match gitiles This change adds a processing pass to md_browser's markdown handling so that if a document only has a single H1, the [TOC] plugin will ignore it. This change also adds a "Contents" H2 to better match Gitiles' output. R=jsbell@chromium.org BUG=590280 ========== to ========== Change [TOC] processing in MD browser to match gitiles This change adds a processing pass to md_browser's markdown handling so that if a document only has a single H1, the [TOC] plugin will ignore it. This change also adds a "Contents" H2 to better match Gitiles' output. R=jsbell@chromium.org BUG=590280 Committed: https://crrev.com/ded33c74ac7aea5962940660717346b65db4d9d0 Cr-Commit-Position: refs/heads/master@{#432000} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/ded33c74ac7aea5962940660717346b65db4d9d0 Cr-Commit-Position: refs/heads/master@{#432000} |
