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

Emit warnings for missing JSDoc comments #2960

Merged
merged 8 commits into from Jul 8, 2020
Merged

Conversation

swissspidy
Copy link
Collaborator

Summary

EmitS warnings for missing JSDoc comments

Relevant Technical Choices

  • Makes eslint-plugin-jsdoc usage explicit
    Before it was implicit via the WordPress ESLint plugin
  • Improve jsdoc in tests folder for demonstration purposes
  • Uses TypeScript mode for JSDoc comments
    This makes it easier to eventually adopt TypeScript (Adopting TypeScript #1715)

To-do

User-facing changes

N/A

Testing Instructions

Run linter


See #1714

@swissspidy swissspidy added Type: Infrastructure Changes impacting testing infrastructure or build tooling Pod: WP & Infra labels Jul 3, 2020
@github-actions
Copy link
Contributor

github-actions bot commented Jul 3, 2020

Size Change: 0 B

Total Size: 1 MB

ℹ️ View Unchanged
Filename Size Change
assets/css/edit-story.css 269 B 0 B
assets/css/stories-dashboard.css 305 B 0 B
assets/css/web-stories-embed-block.css 515 B 0 B
assets/js/edit-story.js 458 kB 0 B
assets/js/stories-dashboard.js 527 kB 0 B
assets/js/web-stories-embed-block.js 15.7 kB 0 B

compressed-size-action

@codecov
Copy link

codecov bot commented Jul 3, 2020

Codecov Report

Merging #2960 into master will increase coverage by 52.79%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #2960       +/-   ##
===========================================
+ Coverage   28.48%   81.27%   +52.79%     
===========================================
  Files         670      710       +40     
  Lines       11652    11989      +337     
===========================================
+ Hits         3319     9744     +6425     
+ Misses       8333     2245     -6088     
Flag Coverage Δ
#karmatests 59.63% <ø> (+38.57%) ⬆️
#unittests 66.71% <ø> (?)
Impacted Files Coverage Δ
...s/src/edit-story/app/media/utils/createResource.js 100.00% <ø> (+100.00%) ⬆️
...ts/src/dashboard/app/views/shared/storyListView.js 24.24% <0.00%> (-0.76%) ⬇️
bin/utils/getCurrentVersionNumber.js 100.00% <0.00%> (ø)
assets/src/edit-story/components/switch/index.js 0.00% <0.00%> (ø)
bin/utils/getIgnoredFiles.js 100.00% <0.00%> (ø)
assets/src/story-embed-block/block/embedLoading.js 100.00% <0.00%> (ø)
bin/utils/updateVersionNumbers.js 100.00% <0.00%> (ø)
assets/src/edit-story/utils/useWhyDidYouUpdate.js 0.00% <0.00%> (ø)
...ts/src/dashboard/storybookUtils/ampStoryWrapper.js 0.00% <0.00%> (ø)
...ets/src/edit-story/app/media/media3p/useMedia3p.js 100.00% <0.00%> (ø)
... and 550 more

@spacedmonkey
Copy link
Contributor

spacedmonkey commented Jul 3, 2020

There are 855 problems (0 errors, 855 warnings) in the linter here. Are we going to fix these now?

@swissspidy
Copy link
Collaborator Author

@spacedmonkey Where are there errors? I only see warnings, and yes they are meant to be fixed - slowly, in multiple PRs, until there are zero warnings left.

@spacedmonkey
Copy link
Contributor

I meant warning yes.

But it mind of breaks github, as it can only highligh changes on 50 lines. Do you think that is a problem?

@spacedmonkey
Copy link
Contributor

Could we generate js docs, in an IDE. Even if they dont have comments? I know phpstorm can generate phpdocs...

@swissspidy
Copy link
Collaborator Author

Ah, you mean the limit of 50 annotations per PR? Yeah that might cause more important messages (errors) to be hidden.

We could skip warnings from being output on GitHub, no big deal. An alternative would be only enabling these warnings for one area of the code base at the time.

Could we generate js docs, in an IDE. Even if they dont have comments? I know phpstorm can generate phpdocs...

Yes and no.

Auto-generated JSDoc annotations are basically like not having any annotations at all. Especially because the IDE can't add type information (because we don't have any types).

Also, JSDoc comments become more useful if they have clear descriptions.

Auto-generated comments can certainly help initially, but all this can only really be done manually.

Copy link
Contributor

@spacedmonkey spacedmonkey left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@cvolzke4 cvolzke4 left a comment

Choose a reason for hiding this comment

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

I notice this is suggesting JSDoc comments on every function, even functions that are private to the file, which may be overkill.

I think we should be careful to avoid comments that mostly repeat the code it's trying to document. If the code is self-explanatory and there's really nothing to document about, and a comment is just redundant and adds unnecessary clutter to the code.

go/java-practices/comments#what_for

@swissspidy
Copy link
Collaborator Author

I notice this is suggesting JSDoc comments on every function, even functions that are private to the file, which may be overkill.

Good point. Just realized the ESLint plugin supports a publicOnly option. Definitely something we can add.

I think we should be careful to avoid comments that mostly repeat the code it's trying to document. If the code is self-explanatory and there's really nothing to document about, and a comment is just redundant and adds unnecessary clutter to the code.

Absolutely. One thing that I want to note is that this change here paves the way for more easily introducing TypeScript in the future. That's because TypeScript can use the JSDoc information for the type checking. So once we have correct types in JSDoc everywhere, switching to TS will be a breeze because we've already dealt with the types.

@swissspidy swissspidy merged commit d735a1f into master Jul 8, 2020
@swissspidy swissspidy deleted the add/jsdoc-eslint-config branch July 8, 2020 06:54
@swissspidy swissspidy mentioned this pull request Mar 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Infrastructure Changes impacting testing infrastructure or build tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants