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

Add generateApiComponents.ts and tests #1074

Merged
merged 8 commits into from
Mar 25, 2024

Conversation

arnaucasau
Copy link
Collaborator

Part of #1008

This PR is a precursor of #1026, and it creates a new script named generateApiComponents.ts with some tests. This script will have all the logic to generate the MDX components, but, in the meantime, this PR adds some functions that can be tested independently by just receiving, as parameters, a list of props or HTML code.

It also copies over the functions prepareGitHubLink and findByText from processHtml.ts

@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! 🙌

@arnaucasau arnaucasau marked this pull request as draft March 22, 2024 17:53
@arnaucasau arnaucasau marked this pull request as ready for review March 22, 2024 18:15
@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! 🙌

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!

scripts/lib/stringUtils.ts Show resolved Hide resolved
* That's because the owning class will already have a link to the relevant file; it's
* noisy and not helpful to repeat the same link without line numbers for the individual methods.
*/
export function prepareGitHubLink(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not a big deal if this is going to be annoying to implement, but you can switch processHtml.md to use this now, along with findByText. That way we don't duplicate. And you could move the tests too if it's too hard.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, it probably would be good to do this if isn't too much of a pain because it may take a few days to land the follow up PR since it's blocked by the Docker image being updated for staging.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

Comment on lines +111 to +113
componentProps.extraRawSignatures = [
...extraRawSignatures.flatMap((sigProps) => sigProps.rawSignature ?? []),
];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Using flatMap surprises me because rawSignature is a string. Should this be

componentProps.extraRawSignatures = Array.from(
    extraRawSignatures.map((x) => x.signature).filter((sig) => sig !== undefined)
)

Also can extraRawSignatures have a more precise type like string[] or Array<string | undefined>? Seems weird to have all the ComponentProps if we're going to discard them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In reality, rawSignature is a string but could be undefined given that we have API types without it. The problem with using the filter is that we get the type (string | undefined)[] instead of string[] | undefined as in the flatMap.

Also can extraRawSignatures have a more precise type

This is a good idea. I think we could remove the flatMap or the map + filter entirely, but I would like to do it in #1026 as a follow-up because we can simplify the main function by only storing the overloaded signature and not all props. The function addExtraSignatures will make all the work as a black box

return "";
}

const html = `<span class="target" id=''/><p><code>${signatureHtml}</code></p>`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is id an empty string?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's empty because it's not needed, but I was able to remove the span and the paragraph too 👍 . Apparently, we only need to add the code tag. Without the code tag, the signature has an extra apostrophe at the beginning and at the end

scripts/lib/api/generateApiComponents.ts Outdated Show resolved Hide resolved
scripts/lib/api/generateApiComponents.test.ts Outdated Show resolved Hide resolved
scripts/lib/api/generateApiComponents.test.ts Outdated Show resolved Hide resolved
@arnaucasau arnaucasau added this pull request to the merge queue Mar 25, 2024
Merged via the queue into Qiskit:main with commit 3f0855d Mar 25, 2024
2 checks passed
@arnaucasau arnaucasau deleted the AC/add-componenets-tests branch March 25, 2024 14:51
frankharkins pushed a commit to frankharkins/documentation that referenced this pull request Jul 22, 2024
Part of Qiskit#1008

This PR is a precursor of Qiskit#1026, and it creates a new script named
`generateApiComponents.ts` with some tests. This script will have all
the logic to generate the MDX components, but, in the meantime, this PR
adds some functions that can be tested independently by just receiving,
as parameters, a list of props or HTML code.

It also copies over the functions `prepareGitHubLink` and `findByText`
from `processHtml.ts`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants