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

typings: lib/internal/vm.js #50112

Merged

Conversation

GeoffreyBooth
Copy link
Member

Tiny PR to add a little JSDoc.

@GeoffreyBooth GeoffreyBooth added vm Issues and PRs related to the vm subsystem. typings labels Oct 10, 2023
@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Oct 10, 2023
@anonrig anonrig removed the needs-ci PRs that need a full CI run. label Oct 10, 2023
@GeoffreyBooth GeoffreyBooth added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 10, 2023
lib/internal/vm.js Outdated Show resolved Hide resolved
@aduh95
Copy link
Contributor

aduh95 commented Nov 29, 2023

This needs a rebase

@aduh95 aduh95 removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 29, 2023
lib/internal/vm.js Outdated Show resolved Hide resolved
lib/internal/vm.js Outdated Show resolved Hide resolved
@fhinkel
Copy link
Member

fhinkel commented Feb 3, 2024

@GeoffreyBooth thanks so much for opening a PR! To be honest, I don't think the doc annotation adds much in this case given they're just a long form of the variable/function names. Would it be OK if we close this without merging it? Thanks for understanding.

@fhinkel fhinkel closed this Feb 3, 2024
@GeoffreyBooth
Copy link
Member Author

I don't think the doc annotation adds much in this case

The main thing it adds is hinting for the types of arguments in your editor as you work. This was waiting on another PR from @joyeecheung but it might be unblocked now. I'm not sure when I'll have time to rebase but anyone else can finish this.

lib/internal/vm.js Show resolved Hide resolved
lib/internal/vm.js Show resolved Hide resolved
lib/internal/vm.js Show resolved Hide resolved
lib/internal/vm.js Show resolved Hide resolved
lib/internal/vm.js Show resolved Hide resolved
Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

LGTM % one suggestion

lib/internal/vm.js Show resolved Hide resolved
lib/internal/vm.js Show resolved Hide resolved
lib/internal/vm.js Show resolved Hide resolved
lib/internal/vm.js Show resolved Hide resolved
lib/internal/vm.js Show resolved Hide resolved
lib/internal/vm.js Show resolved Hide resolved
Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

LGTM if lint complaints are fixed.

lib/internal/vm.js Outdated Show resolved Hide resolved
lib/internal/vm.js Outdated Show resolved Hide resolved
@GeoffreyBooth
Copy link
Member Author

LGTM if lint complaints are fixed.

Thanks, that looks like it fixed the lint check, if you don’t mind reapproving.

@fhinkel
Copy link
Member

fhinkel commented Feb 5, 2024

I don't think the doc annotation adds much in this case

The main thing it adds is hinting for the types of arguments in your editor as you work. This was waiting on another PR from @joyeecheung but it might be unblocked now. I'm not sure when I'll have time to rebase but anyone else can finish this.

Gotcha! Thanks for following up. LGTM.

@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 Feb 7, 2024
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/50112
✔  Done loading data for nodejs/node/pull/50112
----------------------------------- PR info ------------------------------------
Title      typings: lib/internal/vm.js (#50112)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     GeoffreyBooth:type-internal-compile-function -> nodejs:main
Labels     vm, author ready, typings, commit-queue-squash
Commits    7
 - typings: lib/internal/vm.js
 - @legendecas suggestion
 - lint
 - Apply suggestions from code review
 - Update lib/internal/vm.js
 - Apply suggestions from code review
 - Apply suggestions from code review
Committers 2
 - Geoffrey Booth 
 - GitHub 
PR-URL: https://github.com/nodejs/node/pull/50112
Reviewed-By: Yagiz Nizipli 
Reviewed-By: Chengzhong Wu 
Reviewed-By: Franziska Hinkelmann 
Reviewed-By: Joyee Cheung 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/50112
Reviewed-By: Yagiz Nizipli 
Reviewed-By: Chengzhong Wu 
Reviewed-By: Franziska Hinkelmann 
Reviewed-By: Joyee Cheung 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Tue, 10 Oct 2023 03:48:21 GMT
   ✔  Approvals: 4
   ✔  - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/50112#pullrequestreview-1861179506
   ✔  - Chengzhong Wu (@legendecas) (TSC): https://github.com/nodejs/node/pull/50112#pullrequestreview-1864665265
   ✔  - Franziska Hinkelmann (@fhinkel): https://github.com/nodejs/node/pull/50112#pullrequestreview-1864005359
   ✔  - Joyee Cheung (@joyeecheung) (TSC): https://github.com/nodejs/node/pull/50112#pullrequestreview-1865292289
   ✔  Last GitHub CI 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/7815976165

@legendecas legendecas 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 Feb 7, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 14, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

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

Commit Queue failed
- Loading data for nodejs/node/pull/50112
✔  Done loading data for nodejs/node/pull/50112
----------------------------------- PR info ------------------------------------
Title      typings: lib/internal/vm.js (#50112)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     GeoffreyBooth:type-internal-compile-function -> nodejs:main
Labels     vm, author ready, typings, commit-queue-squash
Commits    3
 - typings: lib/internal/vm.js
 - @legendecas suggestion
 - lint
Committers 1
 - Geoffrey Booth 
PR-URL: https://github.com/nodejs/node/pull/50112
Reviewed-By: Yagiz Nizipli 
Reviewed-By: Chengzhong Wu 
Reviewed-By: Franziska Hinkelmann 
Reviewed-By: Joyee Cheung 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/50112
Reviewed-By: Yagiz Nizipli 
Reviewed-By: Chengzhong Wu 
Reviewed-By: Franziska Hinkelmann 
Reviewed-By: Joyee Cheung 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last approving review:
   ⚠  - typings: lib/internal/vm.js
   ⚠  - @legendecas suggestion
   ⚠  - lint
   ℹ  This PR was created on Tue, 10 Oct 2023 03:48:21 GMT
   ✔  Approvals: 4
   ✔  - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/50112#pullrequestreview-1861179506
   ✔  - Chengzhong Wu (@legendecas) (TSC): https://github.com/nodejs/node/pull/50112#pullrequestreview-1864665265
   ✔  - Franziska Hinkelmann (@fhinkel): https://github.com/nodejs/node/pull/50112#pullrequestreview-1864005359
   ✔  - Joyee Cheung (@joyeecheung) (TSC): https://github.com/nodejs/node/pull/50112#pullrequestreview-1865292289
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-02-17T23:55:37Z: https://ci.nodejs.org/job/node-test-pull-request/57165/
- Querying data for job/node-test-pull-request/57165/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/7949622474

@GeoffreyBooth
Copy link
Member Author

Can someone please reapprove this so it can land?

@GeoffreyBooth GeoffreyBooth added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Feb 19, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 19, 2024
@nodejs-github-bot nodejs-github-bot merged commit 0550bc1 into nodejs:main Feb 19, 2024
58 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 0550bc1

targos pushed a commit that referenced this pull request Feb 19, 2024
PR-URL: #50112
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@marco-ippolito marco-ippolito mentioned this pull request Mar 1, 2024
rdw-msft pushed a commit to rdw-msft/node that referenced this pull request Mar 20, 2024
PR-URL: nodejs#50112
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
richardlau pushed a commit that referenced this pull request Mar 25, 2024
PR-URL: #50112
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
richardlau pushed a commit that referenced this pull request Mar 25, 2024
PR-URL: #50112
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@richardlau richardlau mentioned this pull request Mar 25, 2024
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-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. typings vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants