Skip to content

Commit

Permalink
Allow dedicated page for each method (#543)
Browse files Browse the repository at this point in the history
Part of fixing #542.

## Methods living on dedicated pages

Before Qiskit 0.44, methods lived on dedicated HTML pages. We changed
that in Qiskit 0.44 because it resulted in horrible build-time
performance when migrating `qiskit.org/documentation` from the old
Pytorch theme to the new Furo theme, so that instead methods lived
directly on their class pages.

It looks like the script `updateApiDocs.ts` was trying to emulate
methods living on class pages before Qiskit 0.44 through the file
`mergeClassMembers.ts`. This code attempted to remove the method pages
and instead have them live directly on the class pages.

I believe the original script author had `mergeClassMembers.ts` to try
to avoid performance issues. Until a fix a few weeks ago to the docs
application, having a huge amount of pages would cause the site to load
extremely slowly due to the left side bar being too large. That is now
fixed because the left side bar is loaded dynamically rather than
statically.

## `mergeClassMembers.ts` was causing problems

1. It looks like it wasn't behaving properly, at least some of the time.
We did not exhaustively audit every page, but for example,
https://docs.quantum.ibm.com/api/qiskit/0.33/qiskit.result.Result only
had properties at the top, then a table summarizing all the methods
below, but not the methods actually inlined:

<img width="774" alt="Screenshot 2023-12-21 at 11 43 47 AM"
src="https://github.com/Qiskit/documentation/assets/14852634/576ff54a-763c-484d-bc3b-42068693b0d1">

2. We didn't know about this inlining when setting up qiskit.org
redirects, which do a 1:1 mapping so that dedicated method pages take
you to the dedicated method page on IBM, which was 404ing.

## Solution: allow dedicated method pages

By deleting `mergeClassMembers.ts`, the script will properly generate
dedicated HTML pages for each method.

We took this approach—rather than instead fixing
`mergeClassMembers.ts`—for a few reasons:

1. It allows us to avoid messing with our redirect rules, which are the
1:1 mapping.
2. We've gotten user feedback that the API docs are not as usable as
they could be. We are planning to fix that in
#479, but it won't be
fixed in time for putting out the fire of
#542. In the meantime, it
is more user-friendly to have dedicated method pages for those
historical API docs.

The biggest reason we would not want to take this approach of dedicated
method pages is the possible impact on performance mentioned above. But
that has been fixed already in the IBM application.

## Impact of this PR

No impact when generating docs where methods already live on class
pages, like Qiskit 0.44+.

For earlier Qiskit versions, it results in the method pages being
generated. Those will be added in follow up PRs.

---------

Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>
  • Loading branch information
arnaucasau and Eric-Arellano authored Dec 21, 2023
1 parent 74ce974 commit 9ce5f64
Show file tree
Hide file tree
Showing 3 changed files with 0 additions and 287 deletions.
2 changes: 0 additions & 2 deletions scripts/commands/updateApiDocs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ import { WebCrawler } from "../lib/WebCrawler";
import { downloadImages } from "../lib/downloadImages";
import { generateToc } from "../lib/sphinx/generateToc";
import { SphinxToMdResult } from "../lib/sphinx/SphinxToMdResult";
import { mergeClassMembers } from "../lib/sphinx/mergeClassMembers";
import { flatFolders } from "../lib/sphinx/flatFolders";
import { updateLinks } from "../lib/sphinx/updateLinks";
import { renameUrls } from "../lib/sphinx/renameUrls";
Expand Down Expand Up @@ -309,7 +308,6 @@ async function convertHtmlToMarkdown(
await mkdirp(dir);
}

results = await mergeClassMembers(results);
flatFolders(results);
renameUrls(results);
await updateLinks(results, pkg.transformLink);
Expand Down
131 changes: 0 additions & 131 deletions scripts/lib/sphinx/mergeClassMembers.test.ts

This file was deleted.

154 changes: 0 additions & 154 deletions scripts/lib/sphinx/mergeClassMembers.ts

This file was deleted.

0 comments on commit 9ce5f64

Please sign in to comment.