-
Notifications
You must be signed in to change notification settings - Fork 309
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
Update and fix JSDocs related to JobsManager #469
Conversation
src/management/index.js
Outdated
* | ||
* @method importUsers | ||
* @memberOf module:management.ManagementClient.prototype | ||
* | ||
* @example | ||
* var params = { | ||
* connection_id: '{CONNECTION_ID}', | ||
* users: '{PATH_TO_USERS_FILE}' | ||
* users: '{PATH_TO_USERS_FILE}', | ||
* upsert: true, //optional |
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'd remove these optional properties from the sample snippet and keep them in the params docs below
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.
src/management/index.js
Outdated
* @param {String} data.connectionId Connection for the users insertion. | ||
* @param {String} data.users Path to the users data file. | ||
* @param {String} data.users_json JSON data for the users. | ||
* @param {String} data.upsert OPTIONAL: set to true to upsert users, defaults to false |
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.
the way you tell a property is optional in JSDocs is using []
(see cb
below for an example). Should look like this
[data.users_json]
-> Whether to update users if they already exist (true) or to ignore them (false).[data.upsert]
-> Whether to send a completion email to all tenant owners when the job is finished (true) or not (false).
See that I changed the description of those params and also removed the default values, as those are not present in the official docs at least.
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.
Though I removed the default values as you say, I believe they should be described in the document. This is because the default values are determined by the library's implementation, not by the official API.
Also describe that one of them is mandatory.
Also update description.
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.
thank you!!
@lbalmaceda Thank you for the quick review! Note that I've fixed some existing problems ( |
Yes, I saw that. Thanks :) |
Changes
ManagementClient#importUsers
in conformance with that ofJobsManager#importUsers
.get
in the JSDoc ofJobsManager#errors
.Checklist