Skip to content

[node] Improve consistency of JSDoc since tags #60055

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

Merged
merged 2 commits into from
Apr 26, 2022

Conversation

josh-
Copy link
Contributor

@josh- josh- commented Apr 26, 2022

Please fill in this template.

Select one of these and delete the others:
If changing an existing definition:

@amcasey noticed a @since tag that wasn't consistent with the other JSDoc tags - this PR updates other incorrect tags too.

@josh- josh- requested a review from peterblazejewicz as a code owner April 26, 2022 08:40
@typescript-bot
Copy link
Contributor

typescript-bot commented Apr 26, 2022

@josh- Thank you for submitting this PR!

This is a live comment which I will keep updated.

1 package in this PR

Code Reviews

Because this is a widely-used package, a DT maintainer will need to review it before it can be merged.

You can test the changes of this PR in the Playground.

Status

  • ✅ No merge conflicts
  • ✅ Continuous integration tests have passed
  • ✅ Only a DT maintainer can approve changes without tests

All of the items on the list are green. To merge, you need to post a comment including the string "Ready to merge" to bring in your changes.


Diagnostic Information: What the bot saw about this PR
{
  "type": "info",
  "now": "-",
  "pr_number": 60055,
  "author": "josh-",
  "headCommitOid": "39ad4d92be792922468b8fbae362776ff6d2ba15",
  "mergeBaseOid": "65d953cfe06db5c867630bccb1ad2eadff3fd570",
  "lastPushDate": "2022-04-26T08:37:29.000Z",
  "lastActivityDate": "2022-04-26T20:47:34.000Z",
  "mergeOfferDate": "2022-04-26T12:33:14.000Z",
  "mergeRequestDate": "2022-04-26T20:47:34.000Z",
  "mergeRequestUser": "josh-",
  "hasMergeConflict": false,
  "isFirstContribution": false,
  "tooManyFiles": false,
  "hugeChange": false,
  "popularityLevel": "Critical",
  "pkgInfo": [
    {
      "name": "node",
      "kind": "edit",
      "files": [
        {
          "path": "types/node/async_hooks.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/globals.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/v12/async_hooks.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/v12/globals.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/v12/index.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/v12/inspector.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/v14/async_hooks.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/v14/globals.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/v14/inspector.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/v16/async_hooks.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/v16/globals.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/v16/inspector.d.ts",
          "kind": "definition"
        }
      ],
      "owners": [
        "Microsoft",
        "DefinitelyTyped",
        "jkomyno",
        "alvis",
        "r3nya",
        "btoueg",
        "smac89",
        "touffy",
        "DeividasBakanas",
        "eyqs",
        "Hannes-Magnusson-CK",
        "hoo29",
        "kjin",
        "ajafff",
        "islishude",
        "mwiktorczyk",
        "mohsen1",
        "n-e",
        "galkin",
        "parambirs",
        "eps1lon",
        "SimonSchick",
        "ThomasdenH",
        "WilcoBakker",
        "wwwy3y3",
        "samuela",
        "kuehlein",
        "bhongy",
        "chyzwar",
        "trivikr",
        "yoursunny",
        "qwelias",
        "ExE-Boss",
        "peterblazejewicz",
        "addaleax",
        "victorperin",
        "ZYSzys",
        "NodeJS",
        "LinusU",
        "wafuwafu13"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Critical"
    }
  ],
  "reviews": [
    {
      "type": "approved",
      "reviewer": "peterblazejewicz",
      "date": "2022-04-26T12:32:34.000Z",
      "isMaintainer": true
    }
  ],
  "mainBotCommentID": 1109518313,
  "ciResult": "pass"
}

@typescript-bot typescript-bot added Critical package Untested Change This PR does not touch tests labels Apr 26, 2022
@typescript-bot
Copy link
Contributor

@peterblazejewicz
Copy link
Member

just open thoughts for readers pleasure: AFAIK v1.2.3 is not a proper semantic versioning:
https://semver.org/#is-v123-a-semantic-version
but it's a convention used in very own Node sources:
https://github.com/nodejs/node/blob/606bb521591a29df5401732bfbd19c1e31239ed9/lib/util.js#L83

Copy link
Member

@peterblazejewicz peterblazejewicz left a comment

Choose a reason for hiding this comment

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

LGTM!
@josh- thx!

@typescript-bot typescript-bot added Maintainer Approved Self Merge This PR can now be self-merged by the PR author or an owner labels Apr 26, 2022
@josh-
Copy link
Contributor Author

josh- commented Apr 26, 2022

Thanks @peterblazejewicz 🎉

@josh-
Copy link
Contributor Author

josh- commented Apr 26, 2022

Ready to merge

@typescript-bot typescript-bot merged commit ecdd55d into DefinitelyTyped:master Apr 26, 2022
@SimonSchick
Copy link
Contributor

These tags are auto-generated via script, you should consider normalizing the script instead as it will be removed next time.

Also generally adding the patch version might not be worth-while since features/changes are almost always introduced in major and minor versions.

@peterblazejewicz
Copy link
Member

Thanks Simon!

@josh-
Copy link
Contributor Author

josh- commented Apr 28, 2022

Ah thanks very much Simon, I've just run the script VERSION=17.0.0 npm run generate-docs --prefix scripts/generate-docs and it doesn't appear to have reverted any of these changes (I didn't run it for v12/, v14/ or /v16 though). It looks like node has prefixed all their added and deprecated comments with v in the major.minor.patch format?

For example: https://github.com/nodejs/node/blob/68fb0bf553e2af3e0b61733d29e1e9ba7f73d9b2/doc/api/modules.md?plain=1#L1008-L1028

So perhaps the script doesn't need to be normalised?

@josh-
Copy link
Contributor Author

josh- commented Apr 28, 2022

However I notice that by running the script it undid the change from #59930:

diff --git a/types/node/buffer.d.ts b/types/node/buffer.d.ts
index 18be30d2e9e4468c40c308bc6e6f9ecebd4f1b65..1a26043cde63ee07a805d020c732b7b12f3df18e 100644
--- a/types/node/buffer.d.ts
+++ b/types/node/buffer.d.ts
@@ -443,7 +443,7 @@ declare module 'buffer' {
              * Allocates a new `Buffer` of `size` bytes. If `size` is larger than {@link constants.MAX_LENGTH} or smaller than 0, `ERR_INVALID_ARG_VALUE` is thrown.
              *
              * The underlying memory for `Buffer` instances created in this way is _not_
-             * _initialized_. The contents of the newly created `Buffer` are unknown and _may contain sensitive data_. Use `Buffer.alloc()` instead to initialize`Buffer` instances with zeroes.
+             * _initialized_. The contents of the newly created `Buffer` are unknown and_may contain sensitive data_. Use `Buffer.alloc()` instead to initialize`Buffer` instances with zeroes.
              *
              * ```js
              * import { Buffer } from 'buffer';

so I've opened crosstype/node-html-markdown#34 to try and resolve that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Critical package Maintainer Approved Self Merge This PR can now be self-merged by the PR author or an owner Untested Change This PR does not touch tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants