lib: revokeObjectURL throws error on empty args by DylanTet · Pull Request #50433 · nodejs/node · GitHub

Added a check to see if url wasnt included as an argument which will then throw an error.

Review requested:

@nodejs/url

Can you add a test as well?

@anonrig yessir ill knock that out

@anonrig just added the test for the function

LGTM with some linting fixes

First line of commit message has to be started with imperative verb. For example, url: check argument length of revokeObjectURL. Could you please modify first line of commit message?

Refs: https://github.com/nodejs/node/blob/main/doc/contributing/pull-requests.md#commit-message-guidelines

@DylanTet can you remove the unnecessary commits? The PR was polluted because of them, but in general, it looks great.

@H4ad just removed the other commits, sorry about that, I am still getting better at git :)

@DylanTet Can you rename the message of the first commit?

https://github.com/nodejs/node/actions/runs/6683924004/job/18160770160?pr=50433

It should be less than 50 characters, maybe something like: lib: revokeObjectURL throws error on empty args.

You should also fix the linting errors: https://github.com/nodejs/node/actions/runs/6683924005/job/18160770724?pr=50433

You can test locally using make lint-js or make lint-md.

LGTM with lint errors and commit message title fixed.

Hey @DylanTet, Are you working on this?

@shubham9411 i am, last I remember I updated the PR but it's still waiting review

@DylanTet, I think you just need to rename the first commit message as H4ad suggested above.

@shubham9411 done :)

@DylanTet the first message is still failing the CI, did you forgot to push?

Added a check to see if url wasn't included as an argument which will then throw an error. Fixes: nodejs#50432

CI: https://ci.nodejs.org/job/node-test-pull-request/55978/

CI: https://ci.nodejs.org/job/node-test-pull-request/55998/

CI: https://ci.nodejs.org/job/node-test-pull-request/56016/

Landed in 2f40652

Added a check to see if url wasn't included as an argument which will then throw an error. Fixes: #50432 PR-URL: #50433 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>

nodejs-github-bot added needs-ci PRs that need a full CI run. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Oct 27, 2023

anonrig reviewed Oct 27, 2023

View reviewed changes

Comment thread lib/internal/url.js Outdated

Comment thread lib/internal/url.js Outdated

jasnell reviewed Oct 28, 2023

View reviewed changes

Comment thread test/parallel/test-url-revokeobjecturl.js Outdated

jasnell reviewed Oct 28, 2023

View reviewed changes

Comment thread test/parallel/test-url-revokeobjecturl.js Outdated

jasnell approved these changes Oct 28, 2023

View reviewed changes

DylanTet force-pushed the local-branch branch from 9905966 to 3079e71 Compare October 28, 2023 16:57

deokjinkim reviewed Oct 29, 2023

View reviewed changes

Comment thread lib/internal/url.js Outdated

Comment thread test/parallel/test-url-revokeobjecturl.js Outdated

DylanTet changed the title lib: revokeObjectURL throws error if arg length is 0 in url.js url: check argument length of revokeObjectURL Oct 29, 2023

DylanTet force-pushed the local-branch branch from d9f1ca6 to 42a589e Compare October 29, 2023 14:18

H4ad approved these changes Oct 29, 2023

View reviewed changes

H4ad added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Oct 29, 2023

jasnell reviewed Oct 29, 2023

View reviewed changes

Comment thread test/parallel/test-url-revokeobjecturl.js Outdated

DylanTet changed the title url: check argument length of revokeObjectURL lib: revokeObjectURL throws error on empty args Oct 30, 2023

DylanTet force-pushed the local-branch branch from 42a589e to 3c4cc75 Compare October 30, 2023 02:10

lpinca approved these changes Oct 30, 2023

View reviewed changes

deokjinkim mentioned this pull request Nov 28, 2023

url: revokeObjectURL should throw an error when no parameter is provided #50925

Closed

deokjinkim force-pushed the local-branch branch from 3c4cc75 to fe65184 Compare November 28, 2023 14:17

deokjinkim added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Nov 28, 2023

url: throw error if argument length of revokeObjectURL is 0

5f66408

Added a check to see if url wasn't included as an argument which will then throw an error. Fixes: nodejs#50432

deokjinkim force-pushed the local-branch branch from fe65184 to 5f66408 Compare November 28, 2023 14:21

deokjinkim added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 28, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 28, 2023

himself65 approved these changes Nov 28, 2023

View reviewed changes

H4ad approved these changes Nov 30, 2023

View reviewed changes

H4ad added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 30, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 30, 2023

nodejs-github-bot merged commit 2f40652 into nodejs:main Nov 30, 2023

targos mentioned this pull request Dec 4, 2023

v21.4.0 release proposal #51043

Merged

richardlau mentioned this pull request Mar 25, 2024

v20.12.0 proposal #52212

Merged

lib: revokeObjectURL throws error on empty args by DylanTet · Pull Request #50433 · nodejs/node · GitHub

Uh oh!

Uh oh!

lib: revokeObjectURL throws error on empty args - #50433

lib: revokeObjectURL throws error on empty args#50433nodejs-github-bot merged 1 commit intonodejs:mainnodejs/node:mainfrom DylanTet:local-branchDylanTet/node:local-branchCopy head branch name to clipboard

Conversation

DylanTet commented Oct 27, 2023

Uh oh!

nodejs-github-bot commented Oct 27, 2023

Uh oh!

anonrig left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

DylanTet commented Oct 27, 2023

Uh oh!

DylanTet commented Oct 27, 2023

Uh oh!

Uh oh!

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

deokjinkim left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

H4ad commented Oct 29, 2023

Uh oh!

DylanTet commented Oct 29, 2023

Uh oh!

H4ad commented Oct 29, 2023

Uh oh!

Uh oh!

lpinca left a comment

Choose a reason for hiding this comment

Uh oh!

shubham9411 commented Nov 21, 2023

Uh oh!

DylanTet commented Nov 21, 2023

Uh oh!

shubham9411 commented Nov 21, 2023

Uh oh!

DylanTet commented Nov 21, 2023

Uh oh!

H4ad commented Nov 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Nov 28, 2023

Uh oh!

nodejs-github-bot commented Nov 29, 2023

Uh oh!

nodejs-github-bot commented Nov 30, 2023

Uh oh!

nodejs-github-bot commented Nov 30, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

lib: revokeObjectURL throws error on empty args#50433
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
DylanTet:local-branch

H4ad commented Nov 26, 2023 •
edited
Loading