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

Jump to bottom

zlib: refactor to use primordial instead of <string>.startsWith #36718

Closed
wants to merge 1 commit into from
Closed

zlib: refactor to use primordial instead of <string>.startsWith #36718

wants to merge 1 commit into from

Conversation

rchougule
Copy link
Contributor

@rchougule rchougule commented Jan 1, 2021 •
edited

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the zlib Issues and PRs related to the zlib subsystem. label Jan 1, 2021
Lxxyx
Lxxyx approved these changes Jan 1, 2021
View reviewed changes
Copy link
Member

@Lxxyx Lxxyx left a comment

Choose a reason for hiding this comment

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

LGTM except one nit.

I suggest the first word of the commit message should be an imperative verb, something like zlib: refactor to use primordials instead of StringPrototypeStartsWith. Refs: #36679 (comment)

@rchougule
Copy link
Contributor Author

LGTM except one nit.

I suggest the first word of the commit message should be an imperative verb, something like zlib: refactor to use primordials instead of StringPrototypeStartsWith. Refs: #36679 (comment)

Ah yes. My bad. Addressed in 0665753

@rchougule rchougule changed the title zlib: StringPrototypeStartsWith primordial instead of <string>.startsWith zlib: refactor to use primordial instead of <string>.startsWith Jan 1, 2021
aduh95
aduh95 approved these changes Jan 1, 2021
View reviewed changes
lib/zlib.js
Comment on lines +790 to +792
(key) => (StringPrototypeStartsWith(key, 'BROTLI_PARAM_') ?
constants[key] :
0)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I find it easier to read on one line, but you can leave it as is if you prefer.

Suggested change
(key) => (StringPrototypeStartsWith(key, 'BROTLI_PARAM_') ?
constants[key] :
0)
(key) =>
(StringPrototypeStartsWith(key, 'BROTLI_PARAM_') ? constants[key] : 0)

@yashLadha yashLadha 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. labels Jan 8, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 9, 2021
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@yashLadha
Copy link
Member

Landed in 7a6af02

@yashLadha yashLadha closed this Jan 10, 2021
yashLadha pushed a commit that referenced this pull request Jan 10, 2021
@rchougule @yashLadha
zlib: refactor to use primordial instead of <string>.startsWith
7a6af02 
PR-URL: #36718
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Pooja D P <Pooja.D.P@ibm.com>
@rchougule rchougule deleted the zlib_string_primordials branch January 10, 2021 09:17
danielleadams pushed a commit that referenced this pull request Jan 12, 2021
@rchougule @danielleadams
zlib: refactor to use primordial instead of <string>.startsWith
b6f74b0 
PR-URL: #36718
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Pooja D P <Pooja.D.P@ibm.com>
@danielleadams danielleadams mentioned this pull request Jan 12, 2021
Merged
targos pushed a commit that referenced this pull request May 25, 2021
@rchougule @targos
zlib: refactor to use primordial instead of <string>.startsWith
161e805 
PR-URL: #36718
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Pooja D P <Pooja.D.P@ibm.com>
targos pushed a commit that referenced this pull request May 30, 2021
@rchougule @targos
zlib: refactor to use primordial instead of <string>.startsWith
a6aad06 
PR-URL: #36718
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Pooja D P <Pooja.D.P@ibm.com>
targos pushed a commit that referenced this pull request Jun 5, 2021
@rchougule @targos
zlib: refactor to use primordial instead of <string>.startsWith
0131ecf 
PR-URL: #36718
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Pooja D P <Pooja.D.P@ibm.com>
@targos targos mentioned this pull request Jun 6, 2021
Merged
targos pushed a commit that referenced this pull request Jun 11, 2021
@rchougule @targos
zlib: refactor to use primordial instead of <string>.startsWith
740638d 
PR-URL: #36718
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Pooja D P <Pooja.D.P@ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@Trott Trott Trott approved these changes

@Lxxyx Lxxyx Lxxyx approved these changes

@aduh95 aduh95 aduh95 approved these changes

@PoojaDurgad PoojaDurgad PoojaDurgad 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. zlib Issues and PRs related to the zlib subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@rchougule @nodejs-github-bot @yashLadha @jasnell @Trott @Lxxyx @aduh95 @PoojaDurgad