Conversation
Also check tightened PR_RE against pathname
evanlucas
left a comment
evanlucas
left a comment
There was a problem hiding this comment.
Wow, thanks a lot for the contributions! I left a few comments, but overall looks pretty good.
| } | ||
| } | ||
|
|
||
| function getFixesUrl() { |
review.js
Outdated
| } | ||
|
|
||
| function getCollaborators(cb) { | ||
| function getCollaborators() { |
There was a problem hiding this comment.
I would prefer to keep the callback here vs returning a promise
There was a problem hiding this comment.
Okay, but I wonder what the reason for this is?
There was a problem hiding this comment.
It is strictly preference. I prefer nodebacks over promises. :]
| const body = comment.querySelector('.comment-body') | ||
| if (body && LGTM_RE.test(body.innerHTML)) { | ||
| const paragraphs = comment.querySelectorAll('.comment-body > p') | ||
| if (Array.from(paragraphs).some(p => LGTM_RE.test(p.innerHTML))) { |
There was a problem hiding this comment.
just curious, what's the reason for this change? I find this a lot more difficult to read than the previous implementation.
There was a problem hiding this comment.
In nodejs/node#10657, a LGTM embedded in a <blockquote> (and therefore one that does not belong to the author of the comment) was recognized. This piece of code only looks for direct <p> children of .comment-body, and mitigates that issue.
There was a problem hiding this comment.
ah cool that makes sense.
- Make regex static and more concise - Iterate over RE.exec - Use Map
|
Thanks! |
2 participants
Bug fixes:
#fragmentsAlso some internal simplifications of RegExps, Promises, Maps, etc.