[EDX-149] Add canonical docstring comments #1026
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
55003c3 to
c9b0cd9
Compare
85457de to
b127e90
Compare
542ee54 to
33ef1c4
Compare
Base automatically changed from
EDX-158-generate-docs-with-TypeDoc
to
integration/docs
July 8, 2022 07:32
b127e90 to
65b0c33
Compare
a1583fa to
28c3e0d
Compare
28c3e0d to
bff22c5
Compare
b1c03f5 to
f96010a
Compare
This SDK only supports keys of 128 or 256 bits.
This isn’t part of the canonical descriptions because they describe the type of this property as “platform specific”.
…argument I inferred this behaviour by looking at stats() in src/common/lib/client/rest.ts.
…ms` argument As in d0f79d4.
…stBase In ably-js, the realtime classes inherit from the REST ones, which isn’t the case in the IDL or canonical docs.
It’s not supported by the JS SDK, it appears.
So that we get the nice formatting that emphasises what the canonical docstring says.
Copied from the legacy docstring.
Taken (slightly reworded) from the legacy docstring.
I have some stuff I need to fix up here, so want to mark which ones I need to edit. Generated by running `./docstrings_helper.rb mark-canonical-with-params`.
(The stuff I marked in c781896.) I think that the canonical documentation would benefit from using placeholder param names instead of referring to params by their type, but that would require a corresponding change to the IDL which I’m not going to have time to make right now.
Now that the names are now correct after 47f5f52.
Generated by running `./docstrings_helper.rb mark-legacy-matches-canonical`.
(The stuff I marked in 7fc0c55.) Here, “audited” just means that I read the strings and that they don’t contradict my knowledge of the ably-JS SDK or my broader understanding of how our SDKs work. Definitely not a proper proof-read (for style or content), which is outside the scope of this pull request.
Generated by running `./docstrings_helper.rb mark-canonical-without-legacy`.
As in ecfb3fe, “audited” here means that I read the docstrings and didn’t see any obvious contradictions to my understanding of how ably-js works or how our SDKs more broadly work. It also means that the canonical docstrings do not obviously contradict the legacy docstrings. It does not, however, mean that the canonical docstrings are in any way “better” than the legacy ones. For example, there are definitely places where the legacy docstrings are far more detailed than the canonical ones. But I’ve assumed that to be intentional on the part of the canonical docstrings, and even if it’s not, addressing that isn’t within the scope of this work.
I introduced this in c781896 to help me with the process of adding canonical docstrings en masse. Now that’s done, I don’t need it any more.
b4cdacf to
668200c
Compare
|
@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. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 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
(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/docsintomainLEGACY DOCSTRINGmarkers that this pull request introduces into the.d.tsfiles. 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)..d.tsfiles. The reasons that these might exist are similar to those for theLEGACY DOCSTRINGones. 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.mainintointegration/docsand address merge conflicts and add documentation for things that have been added tomainrecently.