-
Notifications
You must be signed in to change notification settings - Fork 87
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
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.
The objects.inv
change when running npm run regen-apis
on the main branch too
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.
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 != "") { |
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.
if (id.endsWith(pageHeading) && pageHeading != "") { | |
if (pageHeading && id.endsWith(pageHeading)) { |
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.
When is pageHeading ever ""
? Ah, probably unit tests?
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.
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.
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.
I applied the suggestion to the Function component too. We were using the same conditional. Thanks :)
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 thepriorApiType
wasn't defined. The absence of apriorApiType
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 runningnpm 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:
Part of #2331