chartjs / chartjs-plugin-annotation

Annotation plugin for Chart.js
MIT License
606 stars 328 forks source link

Add JSDoc to exported functions #722

Closed stockiNail closed 2 years ago

stockiNail commented 2 years ago

This PR is adding type definitions to exported functions.

it also adds new type to map AnnotationBoxModel, https://github.com/chartjs/chartjs-plugin-annotation/pull/706#discussion_r844025717 @kurkle comment

stockiNail commented 2 years ago

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

kurkle commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

stockiNail commented 2 years ago

I have removed the descriptions, leaving the type definitions.