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

tools: enable jsdoc/require-property-description lint rule #45370

Closed
wants to merge 4 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Nov 8, 2022

No description provided.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/modules
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added esm Issues and PRs related to the ECMAScript Modules implementation. http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory. labels Nov 8, 2022
@Trott Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 8, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 8, 2022
@nodejs-github-bot
Copy link
Collaborator

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 8, 2022
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

I'm not sure what's the point, if we were using those to generate .d.ts that could be used outside of core that would make sense, but as is it's just a duplication of the docs that is likely to get outdated at some point.

@Trott
Copy link
Member Author

Trott commented Nov 10, 2022

I'm not sure what's the point, if we were using those to generate .d.ts that could be used outside of core that would make sense, but as is it's just a duplication of the docs that is likely to get outdated at some point.

The JSDoc stuff was added for the benefit of people coding Node.js core using IDEs. For example, they can mouse over things and get a description of possible options values. Hopefully, one day, the JSDoc comments can be used to generate parts of the docs rather than have the information duplicated, but that will take a big effort by someone motivated. (Looking in @ovflowd's direction...)

@ovflowd
Copy link
Member

ovflowd commented Nov 10, 2022

The JSDoc stuff was added for the benefit of people coding Node.js core using IDEs. For example, they can mouse over things and get a description of possible options values. Hopefully, one day, the JSDoc comments can be used to generate parts of the docs rather than have the information duplicated, but that will take a big effort by someone motivated. (Looking in @ovflowd's direction...)

That is something that I haven't taken into consideration for the API docs as they were designed mostly for the publicly exposed APIs for Node.js,

I know some of the JSdoc would be faced to the public Node.js API's, but it actually never crossed my mind to extract them from the source during build docs build time.

It is something we could do, as in the new proposal, for module documentation, we define the sourceLinks which could also be very useful for "reading" the source file searching for extra docs for that method.

Not sure how complex it would be. I bet we could even use a JSDoc parser for reading those segments.

@Trott Trott added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 12, 2022
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Nov 12, 2022
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/45370
✔  Done loading data for nodejs/node/pull/45370
----------------------------------- PR info ------------------------------------
Title      tools: enable jsdoc/require-property-description lint rule (#45370)
Author     Rich Trott  (@Trott)
Branch     Trott:jsdoc -> nodejs:main
Labels     http, tools, esm, author ready, needs-ci
Commits    4
 - http: add JSDoc property descriptions
 - esm: add JSDoc property descriptions for fetch
 - esm: add JSDoc property descriptions for loader
 - tools: enable jsdoc/require-property-description rule
Committers 1
 - Rich Trott 
PR-URL: https://github.com/nodejs/node/pull/45370
Reviewed-By: Akhil Marsonya 
Reviewed-By: Yagiz Nizipli 
Reviewed-By: Jacob Smith 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/45370
Reviewed-By: Akhil Marsonya 
Reviewed-By: Yagiz Nizipli 
Reviewed-By: Jacob Smith 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Tue, 08 Nov 2022 03:43:27 GMT
   ✔  Approvals: 3
   ✔  - Akhil Marsonya (@marsonya): https://github.com/nodejs/node/pull/45370#pullrequestreview-1171838195
   ✔  - Yagiz Nizipli (@anonrig): https://github.com/nodejs/node/pull/45370#pullrequestreview-1172185195
   ✔  - Jacob Smith (@JakobJingleheimer): https://github.com/nodejs/node/pull/45370#pullrequestreview-1176957120
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2022-11-08T15:03:10Z: https://ci.nodejs.org/job/node-test-pull-request/47790/
- Querying data for job/node-test-pull-request/47790/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
✔  origin/main is now up-to-date
- Downloading patch for 45370
From https://github.com/nodejs/node
 * branch                  refs/pull/45370/merge -> FETCH_HEAD
✔  Fetched commits as ca3ed363ffef..67637f1a3729
--------------------------------------------------------------------------------
[main 62b7ead5a8] http: add JSDoc property descriptions
 Author: Rich Trott 
 Date: Mon Nov 7 19:38:35 2022 -0800
 1 file changed, 22 insertions(+), 22 deletions(-)
[main 58d9257e8c] esm: add JSDoc property descriptions for fetch
 Author: Rich Trott 
 Date: Mon Nov 7 19:39:29 2022 -0800
 1 file changed, 3 insertions(+), 3 deletions(-)
[main a70c6745b8] esm: add JSDoc property descriptions for loader
 Author: Rich Trott 
 Date: Mon Nov 7 19:39:44 2022 -0800
 1 file changed, 7 insertions(+), 7 deletions(-)
[main f5591a2c58] tools: enable jsdoc/require-property-description rule
 Author: Rich Trott 
 Date: Mon Nov 7 19:41:12 2022 -0800
 1 file changed, 1 deletion(-)
   ✔  Patches applied
There are 4 commits in the PR. Attempting autorebase.
Rebasing (2/8)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
http: add JSDoc property descriptions

PR-URL: #45370
Reviewed-By: Akhil Marsonya [email protected]
Reviewed-By: Yagiz Nizipli [email protected]
Reviewed-By: Jacob Smith [email protected]

[detached HEAD 44b064bc9c] http: add JSDoc property descriptions
Author: Rich Trott [email protected]
Date: Mon Nov 7 19:38:35 2022 -0800
1 file changed, 22 insertions(+), 22 deletions(-)
Rebasing (3/8)
Rebasing (4/8)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
esm: add JSDoc property descriptions for fetch

PR-URL: #45370
Reviewed-By: Akhil Marsonya [email protected]
Reviewed-By: Yagiz Nizipli [email protected]
Reviewed-By: Jacob Smith [email protected]

[detached HEAD eb181f9cd3] esm: add JSDoc property descriptions for fetch
Author: Rich Trott [email protected]
Date: Mon Nov 7 19:39:29 2022 -0800
1 file changed, 3 insertions(+), 3 deletions(-)
Rebasing (5/8)
Rebasing (6/8)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
esm: add JSDoc property descriptions for loader

PR-URL: #45370
Reviewed-By: Akhil Marsonya [email protected]
Reviewed-By: Yagiz Nizipli [email protected]
Reviewed-By: Jacob Smith [email protected]

[detached HEAD 881e997d26] esm: add JSDoc property descriptions for loader
Author: Rich Trott [email protected]
Date: Mon Nov 7 19:39:44 2022 -0800
1 file changed, 7 insertions(+), 7 deletions(-)
Rebasing (7/8)
Rebasing (8/8)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
tools: enable jsdoc/require-property-description rule

PR-URL: #45370
Reviewed-By: Akhil Marsonya [email protected]
Reviewed-By: Yagiz Nizipli [email protected]
Reviewed-By: Jacob Smith [email protected]

[detached HEAD a38c5ef6d9] tools: enable jsdoc/require-property-description rule
Author: Rich Trott [email protected]
Date: Mon Nov 7 19:41:12 2022 -0800
1 file changed, 1 deletion(-)

Successfully rebased and updated refs/heads/main.

ℹ Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

https://github.com/nodejs/node/actions/runs/3449380941

@Trott Trott added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Nov 12, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 12, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in ca3ed36...917987c

nodejs-github-bot pushed a commit that referenced this pull request Nov 12, 2022
PR-URL: #45370
Reviewed-By: Akhil Marsonya <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
nodejs-github-bot pushed a commit that referenced this pull request Nov 12, 2022
PR-URL: #45370
Reviewed-By: Akhil Marsonya <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
nodejs-github-bot pushed a commit that referenced this pull request Nov 12, 2022
PR-URL: #45370
Reviewed-By: Akhil Marsonya <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
nodejs-github-bot pushed a commit that referenced this pull request Nov 12, 2022
PR-URL: #45370
Reviewed-By: Akhil Marsonya <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
ruyadorno pushed a commit that referenced this pull request Nov 21, 2022
PR-URL: #45370
Reviewed-By: Akhil Marsonya <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
ruyadorno pushed a commit that referenced this pull request Nov 21, 2022
PR-URL: #45370
Reviewed-By: Akhil Marsonya <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
ruyadorno pushed a commit that referenced this pull request Nov 21, 2022
PR-URL: #45370
Reviewed-By: Akhil Marsonya <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
ruyadorno pushed a commit that referenced this pull request Nov 21, 2022
PR-URL: #45370
Reviewed-By: Akhil Marsonya <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
@ruyadorno ruyadorno mentioned this pull request Nov 24, 2022
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
PR-URL: #45370
Reviewed-By: Akhil Marsonya <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
PR-URL: #45370
Reviewed-By: Akhil Marsonya <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
PR-URL: #45370
Reviewed-By: Akhil Marsonya <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
PR-URL: #45370
Reviewed-By: Akhil Marsonya <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
PR-URL: #45370
Reviewed-By: Akhil Marsonya <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
PR-URL: #45370
Reviewed-By: Akhil Marsonya <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
PR-URL: #45370
Reviewed-By: Akhil Marsonya <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
PR-URL: #45370
Reviewed-By: Akhil Marsonya <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
PR-URL: #45370
Reviewed-By: Akhil Marsonya <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
PR-URL: #45370
Reviewed-By: Akhil Marsonya <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
PR-URL: #45370
Reviewed-By: Akhil Marsonya <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
PR-URL: #45370
Reviewed-By: Akhil Marsonya <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 4, 2023
PR-URL: #45370
Reviewed-By: Akhil Marsonya <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 4, 2023
PR-URL: #45370
Reviewed-By: Akhil Marsonya <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 4, 2023
PR-URL: #45370
Reviewed-By: Akhil Marsonya <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 4, 2023
PR-URL: #45370
Reviewed-By: Akhil Marsonya <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. esm Issues and PRs related to the ECMAScript Modules implementation. http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants