Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor htmlToMd.ts: Split prepareHandlers function #943

Merged
merged 3 commits into from
Mar 1, 2024

Conversation

arnaucasau
Copy link
Collaborator

@arnaucasau arnaucasau commented Mar 1, 2024

Part of #845

This PR refactors the htmlToMd.ts script. It splits the prepareHandlers function into some helper functions to simplify the logic and improve the readability.

@qiskit-bot
Copy link
Contributor

Thanks for contributing to Qiskit documentation!

Before your PR can be merged, it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient. Thanks! 🙌

});
}

function buildDt(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same logic as before but encapsulated in a helper function

Comment on lines -105 to -112
if (node.properties.id && node.properties.className?.includes("target")) {
if (
node.properties.id &&
(node.properties.className?.includes("target") ||
node.children.length === 0)
) {
return [buildSpanId(node.properties.id), ...all(h, node)];
}

if (node.properties.id && node.children.length === 0) {
return buildSpanId(node.properties.id);
}

Copy link
Collaborator Author

@arnaucasau arnaucasau Mar 1, 2024

Choose a reason for hiding this comment

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

The difference between these two ifs was the call to all(h, node). This merge should be safe given that the all method transforms the children of a hast parent to mdast or returns an empty array if no children are found.

Description of the function at https://github.com/syntax-tree/hast-util-to-mdast?tab=readme-ov-file#fields-1

scripts/lib/api/htmlToMd.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Nice! Good call to not have buildDeprecatedAdmonition call buildAdmonition. This is much more direct. Sometimes DRY can go too far, which is why WET exists hahaha (write everything twice)

@arnaucasau arnaucasau added this pull request to the merge queue Mar 1, 2024
Merged via the queue into Qiskit:main with commit d9892bc Mar 1, 2024
2 checks passed
@arnaucasau arnaucasau deleted the AC/refactor-htmlToMd-2 branch March 1, 2024 18:48
frankharkins pushed a commit to frankharkins/documentation that referenced this pull request Jul 22, 2024
Part of Qiskit#845

This PR refactors the `htmlToMd.ts` script. It splits the
`prepareHandlers` function into some helper functions to simplify the
logic and improve the readability.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants