ably-js icon indicating copy to clipboard operation
ably-js copied to clipboard

[EDX-149] Add canonical docstring comments

Open lawrence-forooghian opened this issue 2 years ago • 1 comments

What does this do?

Incorporates the canonical docstrings from https://github.com/ably/canonical-api-reference-prototyping/blob/main/descriptions.md into our TypeScript declarations files.

At the time of writing, this pull request has 478 commits, which means that GitHub won't show you all the commits in the "Commits" view (only the most recent 250). To view them all, see https://github.com/ably/ably-js/compare/integration/docs...EDX-149-add-canonical-docstring-comments.

How to review this pull request

I've structured this pull request (see below) so that it's easy to review commit-by-commit. The commit messages contain lots of explanation of the decisions I’ve made.

Please note that I am seeking review of how I have incorporated the canonical docstrings into ably-js – that is, how I have prepared the JS codebase for adding the canonical docstrings, and then how I edited the canonical docstrings to make sense for JS. I am not seeking review of the original canonical docstrings themselves. If you have comments on the content of the canonical docstrings, please raise them as issues in https://github.com/ably/canonical-api-reference-prototyping.

Structure of this pull request

  1. Commits f889b4b3c2acffae2608f3a7f78ca9364ae619f3 to 6cbd4d5b53ef7eb6763cf05dbeaa940cdc4d62d3 inclusive: Prepare our TypeScript declarations files so that they're ready to have the canonical docstrings added to them. This includes things like splitting a single method signature into multiple overloads so that they can be documented individually.
  2. Commit 8b19aa032982b9f1275673662d6772b30280d6c5: Adding markers to delimit any existing ("legacy") docstrings, so that we can later distinguish them from the canonical docstrings.
  3. Commits 169b9f56690e49f9662e8293b117f5fcda59fcc0 to 51a1219df875bdbd2dd9ecd98c4779c1901926b6 inclusive: Adding the canonical docstrings, verbatim, to the TypeScript declarations files.
  4. Commits edc754c8e51e44b18292db37b4d8ffffabaf849a to 5c1ea03b153cb7ada6aa84baf494aa3f557444e1 inclusive: Fixing what I believe to be mistakes in the canonical docstrings. Specifically, these are mistakes that have blocked me in some way from progressing with this work. At some point, the tech writers will likely incorporate these suggested changes into the canonical docstrings.
  5. Commits b803343a808810cffd3faabf1a5a0570ae16f722 to 023226aac1ff0ad9beaf295eb1f2bb3c4715a332 inclusive: Editing the added canonical docstrings so that they are correct for this SDK.
  6. Commits 7fc0c55d24f02c5392838d5aa0c632a9fea1a8c6 to 668200cc3b22ce54c59951f9a5a543b35c1d4e5a: Checking the edited canonical docstrings and removing the markers that I’d used to help me.

(I made a video explaining this, but it's more aimed at who I thought was going to take this work over from me, which didn't happen in the end. If you're interested – you probably shouldn't be, it's long – then it's here.)

Next steps after merging this pull request but before merging integration/docs into main

  • Address all of the remaining LEGACY DOCSTRING markers that this pull request introduces into the .d.ts files. Some of these outstanding ones are because of things that should have canonical documentation but don't yet (which hopefully will be addressed in the aforementioned v2 of canonical docstrings), and some are ably-js specific things. We should write our own docstrings for the latter (and get tech writers to review them, I think).
  • Address all of the remaining "Not yet documented" docstrings in the .d.ts files. The reasons that these might exist are similar to those for the LEGACY DOCSTRING ones. Also ably-js has quite a few method overloads that are very similar to those described by the canonical documentation, but not quite. We should document those ones, basing our documentation on the canonical strings.
  • Add intro blurb (EDX-207)
  • Merge main into integration/docs and address merge conflicts and add documentation for things that have been added to main recently.

lawrence-forooghian avatar Jul 06 '22 15:07 lawrence-forooghian

@owenpearson this is now ready for re-review. I've addressed your comments and incorporated the latest version of the canonical docstrings. I've also added some review notes to the PR description.

lawrence-forooghian avatar Sep 17 '22 19:09 lawrence-forooghian