Skip to content

os: fix cpus invalid return type jsdoc syntax - #43006

Merged
aduh95 merged 2 commits into
nodejs:masterfrom
himself65:20220508-fix-jsdoc
May 10, 2022
Merged

os: fix cpus invalid return type jsdoc syntax#43006
aduh95 merged 2 commits into
nodejs:masterfrom
himself65:20220508-fix-jsdoc

Conversation

@himself65

Copy link
Copy Markdown
Member

figure 1

@nodejs-github-botnodejs-github-bot added needs-ci PRs that need a full CI run. os Issues and PRs related to the os subsystem. labels May 8, 2022
@himself65himself65 added the fast-track PRs that do not need to wait for 48 hours to land. label May 8, 2022
@github-actions

Copy link
Copy Markdown
Contributor

Fast-track has been requested by @himself65. Please 👍 to approve.

@RaisinTenRaisinTen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This syntax is used in a couple of other places too, like

node/lib/os.js

Lines 331 to 336 in 58d8d69

* @returns {{
* uid: number
* gid: number
* username: string
* homedir: string
* shell: string | null
. We should probably have a linter rule to enforce this in

node/.eslintrc.js

Lines 304 to 314 in 58d8d69

// JSDoc recommended rules that we disable
'jsdoc/require-jsdoc': 'off',
'jsdoc/require-param-description': 'off',
'jsdoc/newline-after-description': 'off',
'jsdoc/require-returns-description': 'off',
'jsdoc/valid-types': 'off',
'jsdoc/no-undefined-types': 'off',
'jsdoc/require-param': 'off',
'jsdoc/check-tag-names': 'off',
'jsdoc/require-returns': 'off',
'jsdoc/require-property-description': 'off',
, otherwise future changes might unintentionally undo your change.

@himself65
himself65force-pushed the 20220508-fix-jsdoc branch from 08110f4 to 075b04fCompareMay 8, 2022 16:30
@himself65

Copy link
Copy Markdown
MemberAuthor

This syntax is used in a couple of other places too, like

node/lib/os.js

Lines 331 to 336 in 58d8d69

* @returns {{
* uid: number
* gid: number
* username: string
* homedir: string
* shell: string | null

. We should probably have a linter rule to enforce this in

node/.eslintrc.js

Lines 304 to 314 in 58d8d69

// JSDoc recommended rules that we disable
'jsdoc/require-jsdoc': 'off',
'jsdoc/require-param-description': 'off',
'jsdoc/newline-after-description': 'off',
'jsdoc/require-returns-description': 'off',
'jsdoc/valid-types': 'off',
'jsdoc/no-undefined-types': 'off',
'jsdoc/require-param': 'off',
'jsdoc/check-tag-names': 'off',
'jsdoc/require-returns': 'off',
'jsdoc/require-property-description': 'off',

, otherwise future changes might unintentionally undo your change.

fixed

@himself65

Copy link
Copy Markdown
MemberAuthor

I ran the make lint-js-fix, and there were many codes breaks the rule, for example.

So I don't think we should enable the jsdoc type check for now

For example:

/Users/himself65/Code/node/lib/util.js
85:1 warning Syntax error in type: arg is boolean jsdoc/valid-types
94:1 warning Syntax error in type: arg is null jsdoc/valid-types
103:1 warning Syntax error in type: arg is (null | undefined) jsdoc/valid-types
112:1 warning Syntax error in type: arg is number jsdoc/valid-types
120:1 warning Syntax error in type: arg is string jsdoc/valid-types
129:1 warning Syntax error in type: arg is symbol jsdoc/valid-types
138:1 warning Syntax error in type: arg is undefined jsdoc/valid-types
147:1 warning Syntax error in type: a is NonNullable<object> jsdoc/valid-types
156:1 warning Syntax error in type: arg is Error jsdoc/valid-types
165:1 warning Syntax error in type: arg is Function jsdoc/valid-types
174:1 warning Syntax error in type: arg is (boolean | null | number | string | symbol | undefined) jsdoc/valid-types
209:1 warning Syntax error in type: (...args: any[]) => void jsdoc/valid-types
259:1 warning Syntax error in type: S extends null ? T : (T & S) jsdoc/valid-types
285:1 warning @template should not have a bracketed typein"jsdoc" mode jsdoc/valid-types
287:1 warning Syntax error in type: T extends (...args: infer TArgs) => Promise<infer TReturn>?((...params: [...TArgs, ((err: Error, ret: TReturn) => any)]) => void) : never jsdoc/valid-types

@himself65

Copy link
Copy Markdown
MemberAuthor

I check the eslint-jsdoc rules, and if we wanna use typescript type decelation.

we should set

settings: {jsdoc: {mode: 'typescript'}},

@RaisinTen

Copy link
Copy Markdown
Member

I ran the make lint-js-fix, and there were many codes breaks the rule, for example.

So I don't think we should enable the jsdoc type check for now

If the problem is about adding one more revision to the git blame, I think we could still do the change and add the commit to the .git-blame-ignore-revs file. For reference, Electron does that in https://github.com/electron/electron/blob/main/.git-blame-ignore-revs.

cc @targos since you mentioned the .git-blame-ignore-revs file in #42752 (comment).

Comment threadlib/os.js Outdated
Comment threadlib/os.js
Comment on lines +122 to +123
* model: string,
* speed: number,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I have a personal preference of using ; in type definitions, but I'm fine with , if we already use it elsewhere.

@himself65

himself65 commented May 9, 2022

Copy link
Copy Markdown
MemberAuthor

I've opened a new issue that discusses whether enable jsdoc/valid-types

@aduh95aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 9, 2022
@himself65himself65 added the commit-queue Add this label to land a pull request using GitHub Actions. label May 10, 2022
@nodejs-github-botnodejs-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 May 10, 2022
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator
Commit Queue failed
- Loading data for nodejs/node/pull/43006
✔ Done loading data for nodejs/node/pull/43006
----------------------------------- PR info ------------------------------------
Title os: fix `cpus` invalid return type jsdoc syntax (#43006)
Author Himself65 (@Himself65)
Branch Himself65:20220508-fix-jsdoc -> nodejs:master
Labels os, fast-track, author ready, needs-ci, typings
Commits 2
- os: fix `cpus` invalid return type jsdoc syntax
- fixup!os: fix `cpus` invalid return type jsdoc syntax
Committers 1
- Himself65 PR-URL: https://github.com/nodejs/node/pull/43006
Reviewed-By: Antoine du Hamel Reviewed-By: Darshan Sen ------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/43006
Reviewed-By: Antoine du Hamel Reviewed-By: Darshan Sen --------------------------------------------------------------------------------
ℹ This PR was created on Sun, 08 May 2022 05:08:56 GMT
✔ Approvals: 2
✔ - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/43006#pullrequestreview-966523299
✔ - Darshan Sen (@RaisinTen) (TSC): https://github.com/nodejs/node/pull/43006#pullrequestreview-967159779
ℹ This PR is being fast-tracked
✖ The fast-track request requires at least two collaborators' approvals (👍).
✖ Last GitHub CI failed
✖ No Jenkins CI runs detected
--------------------------------------------------------------------------------
✔ Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/2303698988

@aduh95aduh95 removed fast-track PRs that do not need to wait for 48 hours to land. commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels May 10, 2022
@aduh95
aduh95 merged commit d62e412 into nodejs:masterMay 10, 2022
@aduh95

Copy link
Copy Markdown
Contributor

Landed in d62e412

@himself65
himself65 deleted the 20220508-fix-jsdoc branch May 11, 2022 00:00
BethGriggs pushed a commit that referenced this pull request May 16, 2022
PR-URL: #43006
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
@RafaelGSSRafaelGSS mentioned this pull request May 16, 2022
juanarbol pushed a commit that referenced this pull request May 31, 2022
PR-URL: #43006
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
danielleadams pushed a commit that referenced this pull request Jun 27, 2022
PR-URL: #43006
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
targos pushed a commit that referenced this pull request Jul 12, 2022
PR-URL: #43006
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
targos pushed a commit that referenced this pull request Jul 31, 2022
PR-URL: #43006
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
@targostargos mentioned this pull request Aug 3, 2022
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
PR-URL: nodejs/node#43006
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.needs-ciPRs that need a full CI run.osIssues and PRs related to the os subsystem.typings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@himself65@RaisinTen@nodejs-github-bot@aduh95@targos