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 JSDoc to exported functions #722

Merged
merged 3 commits into from
Apr 13, 2022
Merged

Add JSDoc to exported functions #722

merged 3 commits into from
Apr 13, 2022

Conversation

stockiNail
Copy link
Collaborator

@stockiNail stockiNail commented Apr 8, 2022

This PR is adding type definitions to exported functions.

it also adds new type to map AnnotationBoxModel, #706 (comment) @kurkle comment

@stockiNail stockiNail added documentation types Typescript type changes and removed types Typescript type changes labels Apr 8, 2022
@stockiNail stockiNail added this to the 2.0.0 milestone Apr 8, 2022
@stockiNail
Copy link
Collaborator Author

@kurkle what do you think to add JSDoc to the classes and public methods?

@kurkle
Copy link
Member

kurkle commented Apr 11, 2022

I'm slightly against these, because they tend to get out of date fast (and are a maintenance burden).
For public API it would be ok, but we are only exporting the plugin publicly, right?

I'd be ok switching to typescript though :)

@stockiNail
Copy link
Collaborator Author

@kurkle understood but for my understanding, that means to leave ONLY the param types and remove the rest (description)? Or does it mean to remove all?

@kurkle
Copy link
Member

kurkle commented Apr 11, 2022

@kurkle understood but for my understanding, that means to leave ONLY the param types and remove the rest (description)? Or does it mean to remove all?

All options are ok for me actually. Do these help you maintain the code somehow?

@kurkle
Copy link
Member

kurkle commented Apr 11, 2022

I think this comes from my suggestion of creating a own type for AnnotationBoxModel

What I wanted was this:

from

/**
 * @param {Chart} chart
 * @param {CoreAnnotationOptions} options
 * @returns {{x:number, y: number, x2: number, y2: number, centerX: number, centerY: number, width: number, height: number}}
 */

to

/**
 * @param {Chart} chart
 * @param {CoreAnnotationOptions} options
 * @returns {AnnotationBoxModel}
 */

I'd be fine to remove all the jsdoc too. But very useless descriptions are very useless, like these:
@param {Chart} chart - chart instance
@param {CoreAnnotationOptions} options - annotation options
@returns {AnnotationBoxModel} - a annotation box model

The function descriptions might be useful in some cases, up to you if you want to keep them.

@stockiNail
Copy link
Collaborator Author

We can remove all and maintains the TS links. Nevertheless it's not clear to me why in some cases we added JsDoc (see rotate function in helpers.geometric). Is there any rule? If not, I'm going to remove everywhere in in order to be "aligned". ;)

@kurkle
Copy link
Member

kurkle commented Apr 11, 2022

I don't know if there is a rule, but lets create one :)
If the function name describes all it does, it does not need any more explanation. If however it does not and you'd be required to examine the code to know what it actually does, then it deserves a some jsdoc.

In either case, the types of parameters and return value are helpful. But instead of writing those as jsdoc everywhere, it would be more maintainable to write in typescript.

This would be a good spot to convert (to typescript), but it requires some research and I don't have the time to do it.

The codebase is quite small, so the overhead of jsdoc is quite small too. So there are many things to consider and many good ways to go. No single right answer.

The maintenance overhead snd the tendency to be misaligned with the actual code are the reasons I don't like jsdocs. When they are aligned, they are helpful, this is why I like them.

Not sure if this helps at all :)

@stockiNail
Copy link
Collaborator Author

I fully agree! let me review the PR, leaving what make sense for me and remove where it doesn't make sense.

@stockiNail
Copy link
Collaborator Author

I have removed the descriptions, leaving the type definitions.

@stockiNail stockiNail merged commit 5d9042c into chartjs:master Apr 13, 2022
@stockiNail stockiNail deleted the jsdocFunctions branch April 13, 2022 07:31
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

2 participants