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.

Sign up for GitHub

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

Jump to bottom

lib: revokeObjectURL throws error on empty args #50433

Merged
merged 1 commit into from
Nov 30, 2023
Merged

lib: revokeObjectURL throws error on empty args #50433

merged 1 commit into from
Nov 30, 2023

Conversation

DylanTet
Copy link
Contributor

Fixes: #50432

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

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/url

@nodejs-github-bot 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
anonrig reviewed Oct 27, 2023
View reviewed changes
Copy link
Member

@anonrig anonrig left a comment

Choose a reason for hiding this comment

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

Can you add a test as well?

lib/internal/url.js Outdated Show resolved Hide resolved
lib/internal/url.js Outdated Show resolved Hide resolved
@DylanTet
Copy link
Contributor Author

@anonrig yessir ill knock that out

@DylanTet
Copy link
Contributor Author

@anonrig just added the test for the function

jasnell
jasnell approved these changes Oct 28, 2023
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM with some linting fixes

deokjinkim
deokjinkim reviewed Oct 29, 2023
View reviewed changes
Copy link
Contributor

@deokjinkim deokjinkim left a comment

Choose a reason for hiding this comment

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

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

lib/internal/url.js Outdated Show resolved Hide resolved
test/parallel/test-url-revokeobjecturl.js Outdated Show resolved Hide resolved
@DylanTet 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
@H4ad
Copy link
Member

H4ad commented Oct 29, 2023

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

@DylanTet
Copy link
Contributor Author

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

@H4ad H4ad added author ready PRs that have at least one approval, no pending requests for changes, 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
@H4ad
Copy link
Member

H4ad commented Oct 29, 2023

@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.

@DylanTet DylanTet changed the title url: check argument length of revokeObjectURL lib: revokeObjectURL throws error on empty args Oct 30, 2023
lpinca
lpinca approved these changes Oct 30, 2023
View reviewed changes
Copy link
Member

@lpinca lpinca left a comment

Choose a reason for hiding this comment

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

LGTM with lint errors and commit message title fixed.

@shubham9411
Copy link
Contributor

Hey @DylanTet, Are you working on this?

@DylanTet
Copy link
Contributor Author

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

@shubham9411
Copy link
Contributor

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

@DylanTet
Copy link
Contributor Author

@shubham9411 done :)

@H4ad
Copy link
Member

H4ad commented Nov 26, 2023 •
edited
Loading

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

@deokjinkim deokjinkim mentioned this pull request Nov 28, 2023
Closed
@deokjinkim 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
@DylanTet @deokjinkim
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 deokjinkim added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 28, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 28, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@H4ad H4ad added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 30, 2023
@nodejs-github-bot 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 nodejs-github-bot merged commit 2f40652 into nodejs:main Nov 30, 2023
53 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 2f40652

targos pushed a commit that referenced this pull request Dec 4, 2023
@DylanTet @targos
url: throw error if argument length of revokeObjectURL is 0
6a087ce 
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>
@targos targos mentioned this pull request Dec 4, 2023
Merged
richardlau pushed a commit that referenced this pull request Mar 25, 2024
@DylanTet @richardlau
url: throw error if argument length of revokeObjectURL is 0
2acbcbd 
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>
@richardlau richardlau mentioned this pull request Mar 25, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deokjinkim deokjinkim deokjinkim left review comments

@anonrig anonrig anonrig left review comments

@jasnell jasnell jasnell approved these changes

@himself65 himself65 himself65 approved these changes

@H4ad H4ad H4ad approved these changes

@lpinca lpinca lpinca approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run. whatwg-url Issues and PRs related to the WHATWG URL implementation.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

URL: URL.revokeObjectURL accepts no parameters when 1 is required
9 participants
@DylanTet @nodejs-github-bot @H4ad @shubham9411 @jasnell @lpinca @anonrig @deokjinkim @himself65