Navigation Menu

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

Use lowercase for primitives in jsdocs #6033

Merged
merged 18 commits into from Feb 11, 2019

Conversation

benmccann
Copy link
Contributor

No description provided.

@etimberg
Copy link
Member

Is this recommended by JSDocs?

@benmccann
Copy link
Contributor Author

I don't think jsdoc necessarily cares. I was mostly doing this for consistency with #6020

Typescript cares, so this may make it easier to support in the future: https://www.typescriptlang.org/docs/handbook/declaration-files/do-s-and-don-ts.html

Copy link
Member

@kurkle kurkle left a comment

Choose a reason for hiding this comment

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

Didn't go through all, but I think Object should still be capitalized. Also noticed there are issues with the jsdocs, so maybe it would better to remove all but

/**
 * @private
 * /

from private functions.

I don't have any strong feelings about these, so this review should be regarded as a comment rather than a request for changes.

src/controllers/controller.bar.js Outdated Show resolved Hide resolved
src/core/core.layouts.js Outdated Show resolved Hide resolved
src/core/core.plugins.js Outdated Show resolved Hide resolved
@benmccann
Copy link
Contributor Author

Our docs use lowercase object as does typescript, so I think lowercase would be more consistent

simonbrunel
simonbrunel previously approved these changes Feb 2, 2019
@nagix
Copy link
Contributor

nagix commented Feb 2, 2019

// @param {rectangle} chartArea : the area of the chart to draw full grid lines on
Is rectangle also an interface?

@benmccann
Copy link
Contributor Author

@nagix I fixed the rectangle reference

src/core/core.scale.js Outdated Show resolved Hide resolved
src/core/core.scale.js Outdated Show resolved Hide resolved
@nagix
Copy link
Contributor

nagix commented Feb 6, 2019

There are a few other lines that have JSDoc-style comments but start with //. Shouldn't they also be fixed?

@benmccann
Copy link
Contributor Author

Ok. I've changed all the rest as well. Thanks for catching that

@simonbrunel simonbrunel added this to the Version 2.8 milestone Feb 6, 2019
@nagix
Copy link
Contributor

nagix commented Feb 6, 2019

I realized that those blocks all don't have types. Types are not required in JSDoc, but it might be better to include for consistency (and : should be replaced with -).

@benmccann
Copy link
Contributor Author

benmccann commented Feb 6, 2019

Ok, I've gone through and replaced the : as well. I will leave adding more types for later as it's getting to be a bit outside the scope of this PR. I would want to be careful to document the correct type and there's quite a few places where we don't have any @return or @param where we could add more types as well

nagix
nagix previously approved these changes Feb 7, 2019
Copy link
Contributor

@nagix nagix left a comment

Choose a reason for hiding this comment

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

Thanks for all the changes, looks good to me.

kurkle
kurkle previously approved these changes Feb 10, 2019
src/core/core.plugins.js Outdated Show resolved Hide resolved
src/core/core.layouts.js Outdated Show resolved Hide resolved
@benmccann benmccann dismissed stale reviews from kurkle and nagix via d300e72 February 10, 2019 14:15
src/core/core.plugins.js Outdated Show resolved Hide resolved
src/core/core.plugins.js Outdated Show resolved Hide resolved
src/core/core.interaction.js Outdated Show resolved Hide resolved
src/helpers/helpers.canvas.js Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
src/core/core.helpers.js Show resolved Hide resolved
Copy link
Member

@simonbrunel simonbrunel left a comment

Choose a reason for hiding this comment

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

Great work @benmccann!

Copy link
Contributor

@nagix nagix left a comment

Choose a reason for hiding this comment

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

Thanks for all the cleanups!

@simonbrunel simonbrunel merged commit 2f874fd into chartjs:master Feb 11, 2019
@benmccann
Copy link
Contributor Author

Thanks for the reviews!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants