-
Notifications
You must be signed in to change notification settings - Fork 91
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but I only did a brief look. With absence of tests over the code we will need more eyes. Do not merge without extra reviews.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but I only did a brief look. With absence of tests over the code we will need more eyes. Do not merge without extra reviews.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
this.errors = document.errors; | ||
let m; | ||
while ((m = LazyModuleDocumentation.docsRegex.exec(contents)) !== null) { | ||
if (m && m.groups && m.groups.name && m.groups.doc && m.groups.pre) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a late review (I'm still on PTO today) but I just wanted to say that the code here is deeply nested, meaning that it's hard to reason about.
A common way to avoid this problem is to use "guard expressions" that interrupt the control flow early. For example:
if (m && m.groups && m.groups.name && m.groups.doc && m.groups.pre) { | |
if (!m.groups.?name || !m.groups.?doc && !m.groups.?pre) { | |
continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, you could move the conditional into the loop clause.
this._contents = document.toJSON(); | ||
this.errors = document.errors; | ||
let m; | ||
while ((m = LazyModuleDocumentation.docsRegex.exec(contents)) !== null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The whole loop body is under a conditional block. It seems like if the conditional clause evaluates to false
, this will end up being an infinite loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tomaciazek Can you please make a follow-up to address this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's nothing to fix - this is how regex works in JavaScript, and this particular line is straight from documentation. This loop will always terminate unless it's content interacts (in a very particular way) directly with LazyModuleDocumentation.docsRegex
, which it doesn't.
Fixes #39