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

lib,test,tools: use consistent JSDoc types #40989

Merged
merged 1 commit into from
Nov 29, 2021

Conversation

Trott
Copy link
Member

@Trott Trott commented Nov 27, 2021

This could be in preparation of implementing the jsdoc/check-types
ESLint rule.

This could be in preparation of implementing the jsdoc/check-types
ESLint rule.
@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Nov 27, 2021
@Trott
Copy link
Member Author

Trott commented Nov 27, 2021

This is the result of installing and enabling check-types from eslint-plugin-jsdoc. I'd check that into core but unfortunately it adds over 100K lines of code, so...uh...maybe not.

@Trott
Copy link
Member Author

Trott commented Nov 27, 2021

This is the result of installing and enabling check-types from eslint-plugin-jsdoc. I'd check that into core but unfortunately it adds over 100K lines of code, so...uh...maybe not.

(If it's possible to install and use only check-types and not the entire eslint-plugin-jsdoc, that would be fewer lines of code, of course. I haven't looked into that.)

@VoltrexKeyva
Copy link
Member

VoltrexKeyva commented Nov 27, 2021

I think we should change all of the objects in the JSDoc typings to Record (Indicating an object) and provide types as Record<TYPE, TYPE> (Indicating an object with types, e.g. Record<string, number> -> properties as strings (e.g. test: or 'test':, both works with that type) and the property values as numbers), as the object type in JSDoc typings is always shown as the any type, which does not imply an actual object.

@Mesteery
Copy link
Contributor

I think we should change all of the objects in the JSDoc typings to Record (Indicating an empty object)

Record without type arguments is invalid.

@VoltrexKeyva
Copy link
Member

I think we should change all of the objects in the JSDoc typings to Record (Indicating an empty object)

Record without type arguments is invalid.

Yep, didn't mean to explain it that way; updated the comment.

@aduh95
Copy link
Contributor

aduh95 commented Nov 27, 2021

In our docs we are using Object rather than object everywhere. Are we planing to update that to keep the consistency?

@Trott
Copy link
Member Author

Trott commented Nov 27, 2021

In our docs we are using Object rather than object everywhere. Are we planing to update that to keep the consistency?

I think this requires a larger discussion. If we're going to use JSDoc, let's use JSDoc and use it to generate information in our docs. If we've just got some JSDoc comments here and there but they aren't actually serving a purpose, let's remove them. Otherwise, we have to keep information in two places, but only for some APIs, and there's probably no obvious reason for which APIs that would be.

@aduh95
Copy link
Contributor

aduh95 commented Nov 27, 2021

but they aren't actually serving a purpose

I disagree, they are useful for core developers to get useful autocomplete features from their code editor. That being said I agree it would be better to use it to generate the docs, but I don’t think we should remove it if we can’t.

@Trott
Copy link
Member Author

Trott commented Nov 27, 2021

I disagree, they are useful for core developers to get useful autocomplete features from their code editor.

Ah, OK, if that's the primary purpose at the current time, then sure, let's keep them. I'm not concerned about using Object in docs and object in JSDoc, but I understand if others are. I usually like consistency, so I get. I just feel like internal consistency (all docs use the same thing, all JSDoc comments use the same thing) is more important.

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

Trott commented Nov 27, 2021

I'd check that into core but unfortunately it adds over 100K lines of code, so...uh...maybe not.

If #40995 lands, then adding eslint-plugin-jsdoc will add "only" 93K lines, although over 20K of those lines would be the eslint-plugin-jsdoc README file.

@Trott Trott added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 29, 2021
@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 29, 2021
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/40989
✔  Done loading data for nodejs/node/pull/40989
----------------------------------- PR info ------------------------------------
Title      lib,test,tools: use consistent JSDoc types (#40989)
Author     Rich Trott  (@Trott)
Branch     Trott:jsdoc-fix -> nodejs:master
Labels     lib / src, author ready, needs-ci
Commits    1
 - lib,test,tools: use consistent JSDoc types
Committers 1
 - Rich Trott 
PR-URL: https://github.com/nodejs/node/pull/40989
Reviewed-By: Michaël Zasso 
Reviewed-By: Luigi Pinca 
Reviewed-By: Antoine du Hamel 
Reviewed-By: Tobias Nießen 
Reviewed-By: Mohammed Keyvanzadeh 
Reviewed-By: Ruben Bridgewater 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/40989
Reviewed-By: Michaël Zasso 
Reviewed-By: Luigi Pinca 
Reviewed-By: Antoine du Hamel 
Reviewed-By: Tobias Nießen 
Reviewed-By: Mohammed Keyvanzadeh 
Reviewed-By: Ruben Bridgewater 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Sat, 27 Nov 2021 00:19:42 GMT
   ✔  Approvals: 6
   ✔  - Michaël Zasso (@targos) (TSC): https://github.com/nodejs/node/pull/40989#pullrequestreview-817141936
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/40989#pullrequestreview-817156177
   ✔  - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/40989#pullrequestreview-817158923
   ✔  - Tobias Nießen (@tniessen) (TSC): https://github.com/nodejs/node/pull/40989#pullrequestreview-817168406
   ✔  - Mohammed Keyvanzadeh (@VoltrexMaster): https://github.com/nodejs/node/pull/40989#pullrequestreview-817189140
   ✔  - Ruben Bridgewater (@BridgeAR) (TSC): https://github.com/nodejs/node/pull/40989#pullrequestreview-817207135
   ✔  Last GitHub Actions successful
   ✖  No Jenkins CI runs detected
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/1514030857

@Trott Trott added request-ci Add this label to start a Jenkins CI on a PR. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Nov 29, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 29, 2021
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@Trott Trott added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 29, 2021
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 29, 2021
@nodejs-github-bot nodejs-github-bot merged commit 1e8b296 into nodejs:master Nov 29, 2021
@nodejs-github-bot
Copy link
Collaborator

Landed in 1e8b296

@Trott Trott deleted the jsdoc-fix branch November 29, 2021 06:48
danielleadams pushed a commit that referenced this pull request Dec 13, 2021
This could be in preparation of implementing the jsdoc/check-types
ESLint rule.

PR-URL: #40989
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
danielleadams pushed a commit that referenced this pull request Dec 14, 2021
This could be in preparation of implementing the jsdoc/check-types
ESLint rule.

PR-URL: #40989
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 30, 2022
This could be in preparation of implementing the jsdoc/check-types
ESLint rule.

PR-URL: #40989
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
This could be in preparation of implementing the jsdoc/check-types
ESLint rule.

PR-URL: #40989
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@danielleadams danielleadams mentioned this pull request Feb 1, 2022
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. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants