(WIP) [MMB-156] Add documentation comments and generate HTML documentation #191
Conversation
This file contains hidden or 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
lawrence-forooghian
force-pushed
the
MMB-156-add-documentation-comments
branch
9 times, most recently
from
433b0cd to
4d315a8
Compare
github-actions
bot
temporarily deployed
to
staging/pull/191/typedoc
….once
This copies across the change from commit cd4f7b3 in ably-js:
> This functionality is implemented by wrapping the listener argument in
> another listener. This means that the event emitter does not hold a
> reference to the listener argument (other than that held indirectly
> through the wrapper) and so it is not possible to remove this listener
> using `off(..., listener)`.
>
> The client library specification does not specify a version of `once`
> which accepts an array of event names, and we do not advertise it as
> part of the public API. So, I think the simplest thing is to remove this
> functionality.
The removed code in the current commit also had a bug — the second
argument in the removed code `self.off(eventName, this)` was incorrectly
populated and this made the removed code equivalent to
`self.off(eventName)`; that is, it removed _all_ listeners for the given
event name. I believe that the removal of this code accounts for the
increased expected number of calls to context.spy in one of the tests
here. I’m not sure what reasoning led to the previous expected count of
3 (perhaps the expectation was written just based on the number of calls
observed when running the code), but here’s my reasoning for the
expectation of 4 calls:
Before `context.eventEmitter['off']('myEvent', context.spy)`, the
following calls are relevant to context.spy:
- eventEmitter['on'](context.spy);
- eventEmitter['on']('myEvent', context.spy);
- eventEmitter['on'](['myEvent', 'myOtherEvent', 'myThirdEvent'], context.spy);
- eventEmitter['once'](context.spy);
- eventEmitter['once']('myEvent', context.spy);
After `context.eventEmitter['off']('myEvent', context.spy)`, the
following calls are relevant to context.spy:
- eventEmitter['on'](context.spy);
- eventEmitter['on'](['myEvent' /* no longer applies */, 'myOtherEvent', 'myThirdEvent'], context.spy);
- eventEmitter['once'](context.spy);
After `context.eventEmitter['emit']('myEvent', '')`, the following calls
are relevant to context.spy:
- eventEmitter['on'](context.spy);
- eventEmitter['on'](['myEvent' /* no longer applies *\/, 'myOtherEvent', 'myThirdEvent'], context.spy);
And hence calling `context.eventEmitter['emit']('myOtherEvent', '')`
calls context.spy a further two times.
Make them all of the form "EventMap" prefixed by the name of the class that emits them. Note that the use of "EventMap" as opposed to "EventsMap" naming is also consistent with the naming used by TypeScript for the DOM event maps (e.g. [1]). [1] https://github.com/microsoft/TypeScript/blob/f3dad2a07d007c40d260f13d6b087a2d5685d12d/src/lib/dom.generated.d.ts#L2418-L2422
I am shortly going to be adding documentation comments to the codebase, but only for public API. I’ll then use TypeDoc to generate documentation based on these comments. So, to avoid TypeDoc errors of the form "X does not have any documentation" for non-public API, I’m marking the non-public API as `private` or `@internal`, and will configure TypeDoc to ignore any such-marked items. My guessing of what constitutes public API is done based on what’s mentioned in the documentation on ably.com and what’s used in the demo code bundled in this repo.
As mentioned in ad4e916, I’ll be adding TypeDoc to the project. The aim of the change in this commit is to avoid TypeDoc warnings of the form "X (...) is referenced by Y but not included in the documentation."
So that I can write documentation comments for them.
I want to convert the *EventMap types from types to interfaces, but the EventMap type complicates this. So, let’s get rid of it and the accompanying EventKey type. I can’t see a compelling reason for their existence. Removing them also means fewer types for users to understand, and fewer types for us to document.
Change `listener`’s type from Function to EventListener, and make it clearer that an EventListener takes a single argument.
So that I can use {@link} in documentation comments to link to the
documentation of each of their properties.
So that they can be individually documented.
The subscribe, unsubscribe, … etc methods are easier to understand (and easier to document) when thought of as two variants: one which accepts one or more event names, and one which doesn’t. So create these overload signatures.
Make it clear, via the type system, that a listener can access the name of the emitted event via the `this` variable.
The type system tells us that if `listener` is truthy then it is a function. Assuming that we trust the type system (e.g. assuming that we aren’t trying to catch incorrect usage by non-TypeScript users, and in general I see little evidence that we are trying to) then these isFunction checks are unnecessary.
I don’t know what I’ll do with this information yet.
Copied all relevant-seeming documentation from website (at commit 5f9e999) to roughly appropriate places in code. Done: - avatar.textile - cursors.textile - locations.textile - locking.textile - space.textile Skipped: - setup.textile - index.textile
Copied all documentation from that document to appropriate places in code.
In an upcoming commit, I’m going to use Pandoc to convert the website documentation from Textile to Markdown. However, Pandoc’s parser (incorrectly, I think) thinks that the <p> tag belongs to the link’s URL, so split it up.
We’ll use this very shortly.
Ran `npx ts-node convert_website_documentation.ts && npm run format`.
The docs repo has a bunch of CSS for making these stand out visually, but we don’t have any of that when using TypeDoc (I guess I could configure it but don’t really want to put time into it now), so instead just use the tools at our disposal to make these sections stand out visually.
The relative links were designed for content contained within the ably.com/docs site, which the HTML documentation we generate with TypeDoc will not necessarily be.
lawrence-forooghian
force-pushed
the
MMB-156-add-documentation-comments
branch
from
4d315a8 to
4d8414b
Compare
lawrence-forooghian
changed the base branch from
main
to
prepare-for-documentation-comments
This is largely copied from the way we do things in ably-js (as of its commit 76a5475). I’ve made a couple of tweaks to the config, though: - Added all types (except Project, which I have no idea how to document) to requiredToBeDocumented. I took the list from [1]. TODO what about return values? Is there a way to make TypeDoc complain if they’re not documented? - Excluded internal and private stuff from documentation (see ccafd10). [1] https://github.com/TypeStrong/typedoc/blob/f2d2abe054feca91b89c00c33e1d726bbda85dcb/src/lib/models/reflections/kind.ts#L7
So that I can use CI to upload a preview of the documentation.
lawrence-forooghian
force-pushed
the
MMB-156-add-documentation-comments
branch
from
4d8414b to
58266b7
Compare
lawrence-forooghian
force-pushed
the
prepare-for-documentation-comments
branch
9 times, most recently
from
f3523c0 to
562abfe
Compare
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.
Note: This PR is based on top of #197; please review that one first.
I'm opening this PR to get feedback on the approach that I’m taking here, to be sure it's aligned with what those who requested this documentation had in mind.
My remarks here are written with reference to the work done as of commit 58266b7.
The commits fall broadly into the following sequence:
Prepare the codebase to be documented.(This is now in [MMB-156] Prepare for adding documentation comments #197)class-definitions.md{@link}tags)I'm currently working on steps 5, 6 and 7 of the above. For steps 6 and 7, I’m doing this by working through the list of errors emitted by TypeDoc of things that haven't been documented. In addition to this list, I also intend to add
@returnsannotations to all methods (I don't think it's possible to make TypeDoc emit an error for missing@returnsannotations).Current list of TypeDoc errors (click to expand)
Step 7, in particular, is quite time consuming because it requires me to write confidently and accurately about the SDK even though I am not very familiar with it yet.
In the case where there exists both website documentation and
class-definitions.mddocumentation for something, we need to decide what to do — do we combine, get rid of one etc? (My feeling is that in general the website documentation is more useful than that inclass-definitions.md). We might (I think we should) then want to deleteclass-definitions.md, and if so will need to decide what to do with the other Markdown documentation in the repo that links to it.We also need to decide what to do with the few places in the codebase where there already existed a documentation comment before we started adding the above. I haven't looked into when they were written, and don't yet know whether they're accurate and well-written.
After all of the above is done, it would probably be wise for somebody (doesn't have to be me) to do a further review and edit of the documentation (e.g. some of the documentation copied from the website might need further editing to make sense in this new context).
I've been trying to approach this work in a way such that if we decide we need to stop it at some point due to other priorities, the repo will be in a state where the documentation comments are at least somewhat useful, and could be used to generate HTML documentation that is at least somewhat useful.