Skip to content

Fix some JSDoc errors#7026

Merged
etimberg merged 8 commits intochartjs:masterfrom
benmccann:jsdoc-erros
Feb 3, 2020
Merged

Fix some JSDoc errors#7026
etimberg merged 8 commits intochartjs:masterfrom
benmccann:jsdoc-erros

Conversation

@benmccann
Copy link
Contributor

No description provided.

kurkle
kurkle previously approved these changes Jan 30, 2020
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.

some nits

kurkle
kurkle previously approved these changes Jan 30, 2020
@benmccann
Copy link
Contributor Author

Sorry I kept updating this PR. I think this should now be all the JSDoc errors that TypeScript found

etimberg
etimberg previously approved these changes Jan 31, 2020
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.

I think we should get #7029 merged before this (to avoid requiring a rebase there).

}

return 'none';
return undefined;
Copy link

Choose a reason for hiding this comment

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

When the function returns undefined it looks like an error. Why not return null for example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kurkle are you okay with returning null here?

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer not returning anything (same as returning undefined).
There are some good arguments at SO about this.

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 most upvoted answer on the StackOverflow answer says to return null - though that's not the answer the asker chose. null makes sense to me. But I don't really care or think it matters either so I just made it an empty return for now

@benmccann
Copy link
Contributor Author

@kurkle I don't think this is touching anything that #7029 is, so I don't think that should be a blocker. They both make minor changes to helpers.dom.js, but on different lines. I don't think any rebase will be required

@etimberg etimberg merged commit 1c18a74 into chartjs:master Feb 3, 2020
@etimberg etimberg added this to the Version 3.0 milestone Feb 3, 2020
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.

4 participants