-
Notifications
You must be signed in to change notification settings - Fork 55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add JSDoc annotations to ably.d.ts
#897
Add JSDoc annotations to ably.d.ts
#897
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've spent one and a half hours reading this pull request so far, and only got to just over two thirds of the way through. I'm taking the call at this point to stop reviewing and submit the changes I've spotted so far.
While I acknowledge that much of the copy included here has been copied over from elsewhere, my feeling is that more work should have been done to ensure consistency while that was done. Things I'm spotting:
- Poor sentence structures which make even less sense when introduced into this context
- Inconsistency in the way that terms are referenced, in terms of capitalisation and code formatting - e.g. 'token params' vs 'Token Params' vs 'Token params' (may not have seen this one) vs '
TokenParams
' vs 'TokenParam
s' - Lack of context in some places - e.g.:
- 'Ably' prefix on some concepts
- external links where they might help
- consistent specification of units (e.g. milliseconds) for properties
- Repetition - words/terms that need not be repeated in commentary because they're implicit in the property name or type
The reality is that we're very unlikely to find the time, anytime soon, to go back over all these comments and refine/correct them further. Thus, this is an opportunity to get this right and that requires significant time and effort. I think this pull request needs more time invested into it.
This is great work, well done @owenpearson. Personally I think we should land it, bring all this great work to customers, and raise issues to further improve (after all we'll always be improving docs). This PR has been open since Feb, which means the work was done, but customers are not benefitting from it for months now. |
@mattheworiordan Thanks 👍 The EdX pod are actually looking at formalising how we maintain/add docstrings in our SDKs and are gonna start with ably-js next sprint. IIRC the plan is for them to pick up this PR, so I'll leave it to them and hopefully it will be landed soon. |
Just a note that I'll be taking this one over, starting with resolving the merge conflicts. |
This finishes what was started in 023d467.
This reverts a change introduced in 9b69707. In code review, Quintin expressed concerns about introducing a dependency on typings for a third-party library (i.e. the Node standard library) in our typings. I’m a bit out of my depth here in terms of coming up with a better solution, so I’m just going to revert that change.
This long-running feature branch seems to have accumulated a huge diff in package-lock.json compared to `main`, even though the only intentional change to packages that I can see comes in commit f1ab4fe, which adds the eslint-plugin-jsdoc package. I have no idea what is happening in this package-lock.json diff and I would not be comfortable merging it. So, I’m restoring package-lock.json from `main` (specifically cd35d6b) and re-installing eslint-plugin-jsdoc. This gives a _much_ smaller package-lock.json diff.
1fdb6ee
to
d8d6fa3
Compare
I've done the following here:
I'm keen to get this one merged if possible. The merge conflicts took me several hours to resolve and I'd be very glad to not need to do it again. In terms of @QuintinWillison's other review comments:
@owenpearson would you mind taking a quick look at this one to check my changes? |
@owenpearson pointed out that we're going to do a release of this library soon and that it might be odd for a set of docstrings to appear and then suddenly be replaced on the next release. So I'm happy to keep this in a feature branch for now. |
@QuintinWillison I've changed the target branch of this PR to |
These are just the JS-specific ones that might not get changed in the tech writer’s upcoming docstrings revamp.
Resolved @QuintinWillison’s comments (Slack message). |
Resolves #848
@param
or@returns
tags anywhere, we should perhaps consider adding these in future but I didn't feel it to be essential enough to include in this PR since many of them would just be tautological (for example@param options {Options} - an instance of options
doesn't really give the reader any useful information that they wouldn't already get from the TypeScript type annotations)