Skip to content
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 better documentation for bindgen. #5025

Merged
merged 9 commits into from
Nov 9, 2022
Merged

Conversation

gagik
Copy link
Contributor

@gagik gagik commented Oct 20, 2022

What, How & Why?

Adds better documentation for methods and fields of Realm instance classes. I came up with some principles that decisions were made when I was not sure how to organize, prioritising consistency with legacy documentation as well as clear formatting from language server when viewed from VSCode.

  • General prioritisation order of comments: @params > @throws > @returns > @example > @since.
  • All tags start with uppercase i.e. @returns The sum. The only exception is if it is referring to boolean / other references that happen to lowercase, i.e. @returns true if...
  • No tags are followed by a dash (this was in legacy docs and is actually ignored by JSDoc but for sake of consistency this is removed).
  • No tags other than @throws should specify types since this is TypeScript.
  • @throws is followed by Error type (i.e. {Error}) then either If or When.
  • @returns is followed by The / A if applicable. If it returns a boolean it follows the format: @returns true if X, false if not.
  • @since is kept since it would be easier to remove it then add it back.
  • I tried to replace links with italicised bold text i.e. {@link Realm.Bold bold} => ***bold***. This is because the links in i.e. VSCode were not useful and just led to the source code when clicked which did not seem appropriate in this context. In my opinion best developer experience would be for the links to link to the documentation website for the specific function. I did not see italicised bold text used before, so adding this specific styling to some words will hopefully make it easier to go through them and consider re-adding links in the future. I do think there are other instances where a link to doc would be useful so this is not exhaustive.
  • All methods that override built-in JS parent class methods (i.e. forEach, map) have not been commented as they already have built-in documentation that is inherited.

Let me know if any of these "rules" seem unreasonable and I could change it, they are mostly following the old docs.

☑️ ToDos

  • 📝 Changelog entry
  • 📝 Compatibility label is updated or copied from previous entry
  • 📝 Update COMPATIBILITY.md
  • 🚦 Tests
  • 🔀 Executed flexible sync tests locally if modifying flexible sync
  • 📦 Updated internal package version in consuming package.jsons (if updating internal packages)
  • 📱 Check the React Native/other sample apps work if necessary
  • 📝 Public documentation PR created or is not necessary
  • 💥 Breaking label has been applied or is not necessary

If this PR adds or changes public API's:

  • typescript definitions file is updated
  • jsdoc files updated
  • Chrome debug API is updated if API is available on React Native

@cla-bot cla-bot bot added the cla: yes label Oct 20, 2022
* @param {string} path
* @param {any} encryptionKey?
* @returns number
* @param path - The path to the file where the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not at a computer now, but I'm curious if the dash after param name is ignored? If not, it should probably go away.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dash does get ignored but It could be good to make this consistent anyways.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay since old docs have the dash in most places, I will try to make sure params have dashes

* Searches for a Realm object by its primary key.
* @param type - The type of Realm object to search for.
* @param primaryKey - The primary key value of the object to search for.
* @throws If type passed into this method is invalid or if the object type did
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if it make sense to pick an order of these annotations? So @return is always before @throw and perhaps we could add an eslint plugin for that? Just lower the possible variations among these comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way old docs were made, return was always after the throw, so I kept it this way, generally I kept this order:
param > throws > return > since

* @param type - The type of Realm object to search for.
* @param primaryKey - The primary key value of the object to search for.
* @throws If type passed into this method is invalid or if the object type did
* not have a `primaryKey` specified in its {@link ObjectSchema}.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this link resolve correctly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

links seem to work weirdly... they don't really link to meaningful places (they do link to code) so I saw thinking of removing them.

* This describes the options used to create a Realm.App instance.
* @property id - The id of the Atlas App Services application.
* @property baseUrl - The base URL of the Atlas App Services server.
* @property timeout - General timeout (in millisecs) for requests. TODO
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if these fields are going to be added in the future.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most likely.

packages/realm/src/Collection.ts Outdated Show resolved Hide resolved
packages/realm/src/Dictionary.ts Show resolved Hide resolved
packages/realm/src/List.ts Show resolved Hide resolved
* Searches for a Realm object by its primary key.
* @param type - The type of Realm object to search for.
* @param primaryKey - The primary key value of the object to search for.
* @throws If type passed into this method is invalid or if the object type did
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way old docs were made, return was always after the throw, so I kept it this way, generally I kept this order:
param > throws > return > since

packages/realm/src/Realm.ts Outdated Show resolved Hide resolved
@gagik
Copy link
Contributor Author

gagik commented Oct 21, 2022

There are still some changes to be made but seems like good time to discuss things.

@gagik gagik marked this pull request as ready for review October 21, 2022 08:09
* This describes the options used to create a Realm.App instance.
* @prop id - The id of the Atlas App Services application.
* @prop baseUrl - The base URL of the Atlas App Services server.
* @prop timeout - General timeout (in millisecs) for requests. TODOC
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these fields meant to be added in the future? Should I keep them?

packages/realm/src/Dictionary.ts Show resolved Hide resolved
@kraenhansen
Copy link
Member

@gagik if you find a moment, it would be awesome to get this rebased and merged 🤞

@kraenhansen kraenhansen merged commit 756e34e into bindgen Nov 9, 2022
@kraenhansen kraenhansen deleted the gagik/betterdocs branch November 9, 2022 06:23
RedBeard0531 pushed a commit that referenced this pull request Nov 15, 2022
* Add documentation for Realm

* More documentation and changes.

* Remove more links and add docs.

* More format consistency.

* Add documentation doc.

* Minor corrections

* Remove discuss

* Minor improvements

* Update packages/realm/src/OrderedCollection.ts

Co-authored-by: Kræn Hansen <[email protected]>

Co-authored-by: Kræn Hansen <[email protected]>
RedBeard0531 pushed a commit that referenced this pull request Nov 23, 2022
* Add documentation for Realm

* More documentation and changes.

* Remove more links and add docs.

* More format consistency.

* Add documentation doc.

* Minor corrections

* Remove discuss

* Minor improvements

* Update packages/realm/src/OrderedCollection.ts

Co-authored-by: Kræn Hansen <[email protected]>

Co-authored-by: Kræn Hansen <[email protected]>
takameyer pushed a commit that referenced this pull request Dec 28, 2022
* Add documentation for Realm

* More documentation and changes.

* Remove more links and add docs.

* More format consistency.

* Add documentation doc.

* Minor corrections

* Remove discuss

* Minor improvements

* Update packages/realm/src/OrderedCollection.ts

Co-authored-by: Kræn Hansen <[email protected]>

Co-authored-by: Kræn Hansen <[email protected]>
kraenhansen added a commit that referenced this pull request Mar 1, 2023
* Add documentation for Realm

* More documentation and changes.

* Remove more links and add docs.

* More format consistency.

* Add documentation doc.

* Minor corrections

* Remove discuss

* Minor improvements

* Update packages/realm/src/OrderedCollection.ts

Co-authored-by: Kræn Hansen <[email protected]>

Co-authored-by: Kræn Hansen <[email protected]>
kraenhansen added a commit that referenced this pull request Mar 3, 2023
* Add documentation for Realm

* More documentation and changes.

* Remove more links and add docs.

* More format consistency.

* Add documentation doc.

* Minor corrections

* Remove discuss

* Minor improvements

* Update packages/realm/src/OrderedCollection.ts

Co-authored-by: Kræn Hansen <[email protected]>

Co-authored-by: Kræn Hansen <[email protected]>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants