Skip to content

Skip removing nodes in <code>#647

Merged
gijsk merged 2 commits into
mozilla:masterfrom
lr-nr:keep-code-comments
Nov 23, 2020
Merged

Skip removing nodes in <code>#647
gijsk merged 2 commits into
mozilla:masterfrom
lr-nr:keep-code-comments

Conversation

@jakubriedl

Copy link
Copy Markdown
Contributor

Basically all syntax highlighters wrap comments in code blocks in <span class="comment">// some comment</span>.
And because word comment is correctly in REGEXPS.unlikelyCandidates and REGEXPS.negative they get removed. But those should be kept as it is valid content.

This PR fixes the issue by disabling removing of nodes in <code>.

test url: https://v8.dev/blog/emscripten-standalone-wasm

@gijsk gijsk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good in principle, just one nit around the use of _hasAncestorTag. Thanks!

(And apologies for the slowness, things are rather... hectic... both at work and in my own life.)

Comment thread Readability.js Outdated
return false;
}

if (this._hasAncestorTag(node, "code", -2)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why -2 ? In the other change you didn't pass a maxDepth argument at all. It also looks like passing 0 or -2 is functionally equivalent, so this is a bit confusing. Can you clarify, or maybe change to not pass this if it doesn't make a difference? Thanks!

@jakubriedl

Copy link
Copy Markdown
Contributor Author

nw with response time. Thank you for doing open-source projects.

I've removed the maxDepth param. There isn't actually any need to limit that. Was more copy/paste thing from line above.

@gijsk gijsk merged commit 290724c into mozilla:master Nov 23, 2020
@jakubriedl jakubriedl deleted the keep-code-comments branch November 23, 2020 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants