Conversation
src/Algorithm.ts
Outdated
| } | ||
|
|
||
| for (const step of node.querySelectorAll('li')) { | ||
| if (/\ba new Abstract Closure\b/.test(ownTextContent(step))) { |
There was a problem hiding this comment.
There are other phrasings, e.g. "a new Job Abstract Closure".
We have an existing test here. You could factor it out and use it in both places instead of duplicating.
(There's another test here, come to think, which should probably use the "when called" test; that captures non-AC code blocks like read-modify-write modification function as well.)
There was a problem hiding this comment.
The / performs the following steps (atomically )?when called:$/ test seems best to me, and I like the idea of factoring it out for use like
| if (/\ba new Abstract Closure\b/.test(ownTextContent(step))) { | |
| if (isAbstractClosure(step)) { |
e.g.,
const acHeaderRe =
/ performs the following steps (atomically )?when called:$/`;
export function isAbstractClosure(step: Element): boolean {
// Decide based on text of the last non-<ol> child.
const { childNodes } = step;
for (let i = childNodes.length - 1; i >= 0; i--) {
const childNode = childNodes[i];
if ((childNode as Element).tagName === 'OL') continue;
return acHeaderRe.test(childNode.textContent);
}
return false;
}There was a problem hiding this comment.
I'd prefer to have it just run on text instead of elements, and let the caller pick the text to use instead of having to operate on HTML elements of an assumed structure.
There was a problem hiding this comment.
FWIW, the implementation that landed is inherently less efficient than one which looks only at the text of a single child and therefore has a runtime that basically triples the above. I'd prefer processing to be as fast as possible, except where the resulting optimizations hinder comprehension (which I claim is not the case here).
src/Algorithm.ts
Outdated
| } | ||
|
|
||
| for (const step of node.querySelectorAll('li')) { | ||
| if (/\ba new Abstract Closure\b/.test(ownTextContent(step))) { |
There was a problem hiding this comment.
The / performs the following steps (atomically )?when called:$/ test seems best to me, and I like the idea of factoring it out for use like
| if (/\ba new Abstract Closure\b/.test(ownTextContent(step))) { | |
| if (isAbstractClosure(step)) { |
e.g.,
const acHeaderRe =
/ performs the following steps (atomically )?when called:$/`;
export function isAbstractClosure(step: Element): boolean {
// Decide based on text of the last non-<ol> child.
const { childNodes } = step;
for (let i = childNodes.length - 1; i >= 0; i--) {
const childNode = childNodes[i];
if ((childNode as Element).tagName === 'OL') continue;
return acHeaderRe.test(childNode.textContent);
}
return false;
}|
Comments addressed. |
1355cb9 to
a38c06d
Compare
| export function ownTextContent(el: Element): string { | ||
| let text = ''; | ||
| for (const child of el.childNodes) { | ||
| if (child.nodeType === 1 && (child as Element).tagName === 'OL') continue; | ||
| text += child.textContent; | ||
| } | ||
| return text; | ||
| } |
There was a problem hiding this comment.
For the record, I have the same complaint about this function as @bakkot raised about my proposed isAbstractClosure—it presumes an HTML structure without any indication of such a requirement (i.e., "own" does not imply anything specific to HTML list items).
If we're going to keep this function implementation (given an element, return the concatenation of all non-<ol> children's textContent), I'd prefer renaming it to something like stepOwnTextContent, or alternatively generalizing the implementation to also filter out <ul> children and renaming it to something like liOwnTextContent.
Fixes #682.
Disclaimer: I used AI in the process of making this PR.