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 missing attribute headers for API docs #2341

Merged
merged 44 commits into from
Nov 19, 2024

Conversation

arnaucasau
Copy link
Collaborator

@arnaucasau arnaucasau commented Nov 18, 2024

This PR adds missing attribute headers for some versions of Qiskit. To decide if an attribute was on a dedicated page and therefore didn't need a header, the code was checking if the id existed and if the priorApiType wasn't defined. The absence of a priorApiType in an attribute could be seen as indicative of the attribute not being in a class page, but it's not enough to check if that page is dedicated to just one attribute or if there are more.

To solve this, the PR changes the conditional to check if the page heading matches the id of the attribute as we are already doing for the functions' component. I've checked the correctness by running npm run regen-apis and the only changes found are the missing attribute headers for qiskit 0.44.0, 0.45.3, 0.45.3, and 1.0.2.

Furthermore, I tested with the MPF docs, and it solves the issue with the missing attribute:

-<Attribute id="qiskit_addon_mpf.backends.HAS_QUIMB" isDedicatedPage={true}>
+### HAS\_QUIMB
+
+<Attribute id="qiskit_addon_mpf.backends.HAS_QUIMB">

Part of #2331

@arnaucasau arnaucasau marked this pull request as ready for review November 19, 2024 13:26
@arnaucasau arnaucasau changed the title [WIP] Add missing attribute headers for API docs Add missing attribute headers for API docs Nov 19, 2024
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The objects.inv change when running npm run regen-apis on the main branch too

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 work! The objects.inv changes are weird - git diff on my local machine doesn't show any meaningful change even with the objects.inv diff viewer. I wonder if your machine is configured differently, but it's weird that we both use macOS

@@ -252,7 +244,8 @@ function prepareAttributeOrPropertyProps(
modifiers: filteredModifiers,
};

if (!priorApiType && id) {
const pageHeading = $dl.siblings("h1").text();
if (id.endsWith(pageHeading) && pageHeading != "") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if (id.endsWith(pageHeading) && pageHeading != "") {
if (pageHeading && id.endsWith(pageHeading)) {

Copy link
Collaborator

Choose a reason for hiding this comment

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

When is pageHeading ever ""? Ah, probably unit tests?

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 could be an empty string if for some reason (structure of the page) there isn't any <h1> as a sibling. I'm not sure we are testing the case explicitly, but it's implicitly tested in some cases like in this test where it doesn't have a heading.

test("data", async () => {

If we had an <h1> with the same name as the attribute in the test, the result wouldn't include the <h3> because it would enter into the if of line 248.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I applied the suggestion to the Function component too. We were using the same conditional. Thanks :)

@arnaucasau arnaucasau added this pull request to the merge queue Nov 19, 2024
Merged via the queue into Qiskit:main with commit ceaa861 Nov 19, 2024
3 checks passed
@arnaucasau arnaucasau deleted the AC/add-missing-headers branch November 19, 2024 15:12
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.

2 participants