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

lib: move encodeStr function to internal for reusable #24242

Closed
wants to merge 1 commit into from

Conversation

ZYSzys
Copy link
Member

@ZYSzys ZYSzys commented Nov 8, 2018

The encodeStr function and qsEscape function are almost the same.
Extract encodeStr function and move it to internal for reusable.

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

cc @mscdex

@nodejs-github-bot nodejs-github-bot added the querystring Issues and PRs related to the built-in querystring module. label Nov 8, 2018
@mscdex
Copy link
Contributor

mscdex commented Nov 8, 2018

@ZYSzys
Copy link
Member Author

ZYSzys commented Nov 8, 2018

Is the Benchmark CI result acceptable ? 🤔

BTW, didn't understand why we use function

var encode = QueryString.escape;

instead of

var encode = qsEscape;

@mscdex
Copy link
Contributor

mscdex commented Nov 8, 2018

BTW, didn't understand why we use function

var encode = QueryString.escape;

instead of

var encode = qsEscape;

The reason is that we allow users to override the escape function.

@mscdex
Copy link
Contributor

mscdex commented Nov 8, 2018

Is the Benchmark CI result acceptable ?

I suppose so.

@Trott
Copy link
Member

Trott commented Nov 16, 2018

@nodejs/url This doesn't technically need another review, but it would be nice.

@joyeecheung
Copy link
Member

@Trott
Copy link
Member

Trott commented Nov 21, 2018

Landed in db9a745

@Trott Trott closed this Nov 21, 2018
Trott pushed a commit to Trott/io.js that referenced this pull request Nov 21, 2018
PR-URL: nodejs#24242
Reviewed-By: James M Snell <jasnell@gmail.com>
@ZYSzys ZYSzys deleted the encodeStr branch November 21, 2018 02:57
targos pushed a commit that referenced this pull request Nov 21, 2018
PR-URL: #24242
Reviewed-By: James M Snell <jasnell@gmail.com>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
PR-URL: #24242
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Dec 5, 2018
4 tasks
codebytere pushed a commit that referenced this pull request Jan 13, 2019
PR-URL: #24242
Reviewed-By: James M Snell <jasnell@gmail.com>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
PR-URL: nodejs#24242
Reviewed-By: James M Snell <jasnell@gmail.com>
@codebytere codebytere mentioned this pull request Jan 15, 2019
codebytere pushed a commit that referenced this pull request Jan 29, 2019
PR-URL: #24242
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
querystring Issues and PRs related to the built-in querystring module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants