Skip to content

test, url: improve escaping in url.parse #10083

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

test, url: improve escaping in url.parse #10083

wants to merge 1 commit into from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Dec 2, 2016 •
edited
Loading

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test, url

Description of change
  • rename variables in autoEscapeStr so they are easier to understand
  • comment the escaping algorithm
  • increase coverage for autoEscapeStr

@nodejs-github-bot nodejs-github-bot added the url Issues and PRs related to the legacy built-in url module. label Dec 2, 2016
@joyeecheung
test, url: improve escaping in url.parse
60da4b0 
- rename variables in autoEscapeStr so they are easier to understand
- comment the escaping algorithm
- increase coverage for autoEscapeStr
addaleax
addaleax approved these changes Dec 2, 2016
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks, this is awesome!

@addaleax
Copy link
Member

addaleax commented Dec 2, 2016

CI: https://ci.nodejs.org/job/node-test-commit/6316/

@addaleax
Copy link
Member

addaleax commented Dec 4, 2016

Landed in 4422c35, thanks!

@addaleax addaleax closed this Dec 4, 2016
addaleax pushed a commit that referenced this pull request Dec 4, 2016
@joyeecheung @addaleax
test,url: improve escaping in url.parse
4422c35 
- rename variables in autoEscapeStr so they are easier to understand
- comment the escaping algorithm
- increase coverage for autoEscapeStr

PR-URL: #10083
Reviewed-By: Anna Henningsen <anna@addaleax.net>
addaleax pushed a commit that referenced this pull request Dec 5, 2016
@joyeecheung @addaleax
test,url: improve escaping in url.parse
ea1c4e1 
- rename variables in autoEscapeStr so they are easier to understand
- comment the escaping algorithm
- increase coverage for autoEscapeStr

PR-URL: #10083
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@Fishrock123 Fishrock123 mentioned this pull request Dec 5, 2016
Merged
2 tasks
jmdarling pushed a commit to jmdarling/node that referenced this pull request Dec 8, 2016
@joyeecheung
test,url: improve escaping in url.parse
81d9a1a 
- rename variables in autoEscapeStr so they are easier to understand
- comment the escaping algorithm
- increase coverage for autoEscapeStr

PR-URL: nodejs#10083
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Dec 20, 2016
@joyeecheung @MylesBorins
test,url: improve escaping in url.parse
13b1688 
- rename variables in autoEscapeStr so they are easier to understand
- comment the escaping algorithm
- increase coverage for autoEscapeStr

PR-URL: #10083
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
@joyeecheung @MylesBorins
test,url: improve escaping in url.parse
a7c1f38 
- rename variables in autoEscapeStr so they are easier to understand
- comment the escaping algorithm
- increase coverage for autoEscapeStr

PR-URL: #10083
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@MylesBorins MylesBorins mentioned this pull request Dec 21, 2016
Merged
@joyeecheung joyeecheung deleted the improve-url-escape branch January 2, 2017 05:28
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@addaleax addaleax addaleax approved these changes

Assignees
No one assigned
Labels
url Issues and PRs related to the legacy built-in url module.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@joyeecheung @addaleax @MylesBorins @nodejs-github-bot