Make API generation more maintainable and readable #223
Comments
Eric-Arellano
changed the title
Eric-Arellano
changed the title
github-merge-queue bot
pushed a commit
that referenced
this issue
Jan 5, 2024
Part of #223. First small refactor of the `sphinxHtmlToMarkdown.ts` script. This PR divides the `sphinxHtmlToMarkdown` function into different helper functions, reducing its length and allowing the possibility of adding tests for different parts of the code. Changes in this PR: - Renamed the `$page` variable to `$` to reduce the noise introduced by the number of calls we had and to follow the [Cheerio](https://cheerio.js.org/docs/basics/loading#load) convention. - Moved the code to find all the images into a helper function. - Grouped similar transformations to the HTML (done previous to its conversion to markdown) into a helper function called `preprocess`. - Moved the conversion from HTML to markdown process to a different function to simplify the `sphinxHtmlToMarkdown` function (divide more in the future). - Refactored the recursive search of methods, adding early returns and merging some parts. - Moved the unified plugin to the `unifiedParser.ts` file in order to abstract the transformation, which can now be called from other files. That script can group together all the unified plugins, such as the link checker, the mergeClassMembers.ts, and this one. In this PR, the code has been copied almost identically to how it was before, except for some little adjustments, like in the unified plugin where we now use an anonymous function and have the two visitors merged. More changes will be introduced in a follow-up. --------- Co-authored-by: Eric Arellano <[email protected]>
github-merge-queue bot
pushed a commit
that referenced
this issue
Jan 5, 2024
Part of #223. One way to improve the maintainability of our API generation is to test more. It gives us confidence we aren't breaking things and it makes it more obvious how the function behaves. This PR also makes some light refactoring like renaming things to be more clear. --------- Co-authored-by: Arnau Casau <[email protected]> Co-authored-by: Arnau Casau <[email protected]>
github-actions bot
pushed a commit
that referenced
this issue
Jan 9, 2024
Part of #223. First small refactor of the `sphinxHtmlToMarkdown.ts` script. This PR divides the `sphinxHtmlToMarkdown` function into different helper functions, reducing its length and allowing the possibility of adding tests for different parts of the code. Changes in this PR: - Renamed the `$page` variable to `$` to reduce the noise introduced by the number of calls we had and to follow the [Cheerio](https://cheerio.js.org/docs/basics/loading#load) convention. - Moved the code to find all the images into a helper function. - Grouped similar transformations to the HTML (done previous to its conversion to markdown) into a helper function called `preprocess`. - Moved the conversion from HTML to markdown process to a different function to simplify the `sphinxHtmlToMarkdown` function (divide more in the future). - Refactored the recursive search of methods, adding early returns and merging some parts. - Moved the unified plugin to the `unifiedParser.ts` file in order to abstract the transformation, which can now be called from other files. That script can group together all the unified plugins, such as the link checker, the mergeClassMembers.ts, and this one. In this PR, the code has been copied almost identically to how it was before, except for some little adjustments, like in the unified plugin where we now use an anonymous function and have the two visitors merged. More changes will be introduced in a follow-up. --------- Co-authored-by: Eric Arellano <[email protected]>
github-actions bot
pushed a commit
that referenced
this issue
Jan 9, 2024
Part of #223. One way to improve the maintainability of our API generation is to test more. It gives us confidence we aren't breaking things and it makes it more obvious how the function behaves. This PR also makes some light refactoring like renaming things to be more clear. --------- Co-authored-by: Arnau Casau <[email protected]> Co-authored-by: Arnau Casau <[email protected]>
github-merge-queue bot
pushed a commit
that referenced
this issue
Jan 9, 2024
Part of #223. There are still improvements we can make like better testing. This PR tries to avoid making code changes and only moves the code to helper functions.
javabster
modified the milestones:
24-05-21 qiskit 1.0 release part 2,
runtime primitives release
frankharkins
pushed a commit
to frankharkins/documentation
that referenced
this issue
Jul 22, 2024
Part of Qiskit#223. First small refactor of the `sphinxHtmlToMarkdown.ts` script. This PR divides the `sphinxHtmlToMarkdown` function into different helper functions, reducing its length and allowing the possibility of adding tests for different parts of the code. Changes in this PR: - Renamed the `$page` variable to `$` to reduce the noise introduced by the number of calls we had and to follow the [Cheerio](https://cheerio.js.org/docs/basics/loading#load) convention. - Moved the code to find all the images into a helper function. - Grouped similar transformations to the HTML (done previous to its conversion to markdown) into a helper function called `preprocess`. - Moved the conversion from HTML to markdown process to a different function to simplify the `sphinxHtmlToMarkdown` function (divide more in the future). - Refactored the recursive search of methods, adding early returns and merging some parts. - Moved the unified plugin to the `unifiedParser.ts` file in order to abstract the transformation, which can now be called from other files. That script can group together all the unified plugins, such as the link checker, the mergeClassMembers.ts, and this one. In this PR, the code has been copied almost identically to how it was before, except for some little adjustments, like in the unified plugin where we now use an anonymous function and have the two visitors merged. More changes will be introduced in a follow-up. --------- Co-authored-by: Eric Arellano <[email protected]>
frankharkins
pushed a commit
to frankharkins/documentation
that referenced
this issue
Jul 22, 2024
Part of Qiskit#223. One way to improve the maintainability of our API generation is to test more. It gives us confidence we aren't breaking things and it makes it more obvious how the function behaves. This PR also makes some light refactoring like renaming things to be more clear. --------- Co-authored-by: Arnau Casau <[email protected]> Co-authored-by: Arnau Casau <[email protected]>
frankharkins
pushed a commit
to frankharkins/documentation
that referenced
this issue
Jul 22, 2024
Part of Qiskit#223. There are still improvements we can make like better testing. This PR tries to avoid making code changes and only moves the code to helper functions.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Right now, it can be somewhat brittle when e.g. Sphinx changes how HTML is generated, which caused the issues in #67. (Although, Axel did a great job writing tests!)
I think probably this looks like adding more testing where possible.
Originally, I thought we should consider switching from Sphinx to https://vemel.github.io/handsdown/. But we decided this is not a good option, including because cross-references are extremely important for the API docs.
--
Update Jan 3: @frankharkins, @arnaucasau, and I agreed this issue is high priority before we add more complexity via #522, #454, and #479.
Rough plan:
mergeClassMembers.ts,updateLinks.ts, andgenerateToc.tsstill need better testing. For example, I don't thinkgenerateToc.tsis teesting all the differente edge cases adequatelyupdateApiDocs.ts- what can we move tolibso that we can test it?sphinxHtmlToMarkdown.ts, our most complex file. Split out the huge function into smaller helper functions & write more focused unit tests for those. Right now our unit tests are a bit hard to read and update since they have to mock so much.convertHtmlToMarkdown, which right now does a lot. Is there a better level of abstraction?The text was updated successfully, but these errors were encountered: