lib: remove duplicated code #19076
Closed
lib: remove duplicated code #19076
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nodejs-github-bot
added
the
lib / src
daynin
changed the title
mscdex
reviewed
Mar 2, 2018
lib/internal/url.js
Outdated
| const firstEncodedParam = encodeStr(array[0], noEscape, paramHexTable); | ||
| const firstEncodedValue = encodeStr(array[1], noEscape, paramHexTable); | ||
| let output = | ||
| `${firstEncodedParam}=${firstEncodedValue}`; |
mscdex
reviewed
Mar 2, 2018
lib/querystring.js
Outdated
| if (lastPos < str.length) | ||
| return out + str.slice(lastPos); | ||
| return out; | ||
| return encodeStr(str, noEscape, hexTable, { |
There was a problem hiding this comment.
Did you benchmark this change?
starkwang
reviewed
Mar 2, 2018
lib/internal/url.js
Outdated
| @@ -3,7 +3,8 @@ | |||
| const util = require('util'); | |||
| const { | |||
| hexTable, | |||
| isHexTable | |||
| isHexTable, | |||
| encodeStr, | |||
There was a problem hiding this comment.
Unexpected trailing comma. Remember to run lint before commit: make lint
There was a problem hiding this comment.
@starkwang I did it. And everything was ok:
There was a problem hiding this comment.
@starkwang we had this rule for trailing comma
'comma-dangle': ['error', 'only-multiline']
It allows using trailing comma. But maybe we should fix this rule. What do you think?
There was a problem hiding this comment.
Oh sorry, I misunderstood the meaning of 'only-multiline'. Just ignore it.
| @@ -36,7 +36,7 @@ const { | |||
| URLSearchParams, | |||
| domainToASCII, | |||
| domainToUnicode, | |||
| formatSymbol | |||
| formatSymbol, | |||
There was a problem hiding this comment.
@starkwang I'll fix it
There was a problem hiding this comment.
Trailing commas are fine and if we didn't have so much code that lacked them, we would probably even have a lint rule to enforce them. Anything that cleans up git diffs in the long-term is a positive.
|
@mscdex @starkwang I'm running benchmarks now, so I'll attach the results as soon as possible |
apapirovski
reviewed
Mar 2, 2018
| @@ -36,7 +36,7 @@ const { | |||
| URLSearchParams, | |||
| domainToASCII, | |||
| domainToUnicode, | |||
| formatSymbol | |||
| formatSymbol, | |||
There was a problem hiding this comment.
Trailing commas are fine and if we didn't have so much code that lacked them, we would probably even have a lint rule to enforce them. Anything that cleans up git diffs in the long-term is a positive.
lib/querystring.js
Outdated
| if (lastPos < str.length) | ||
| return out + str.slice(lastPos); | ||
| return out; | ||
| return encodeStr(str, noEscape, hexTable, { |
There was a problem hiding this comment.
Can you predeclare the error type somewhere at the top scope? There's no need to create this object on the run each time.
|
@mscdex @starkwang you were right about "querystring" module. There is performance reduction in this change: I'll revert changes for "querystring" and run benchmarks for "url" |
daynin
force-pushed
the
remove-duplicated-code
branch
from
d6f2dd1 to
43b60af
Compare
|
I reverted changes for "querystring" module and ran benchmarks for "url" module only. Results: Looks like these changes make the code a little bit slower, what do you think about it? |
BridgeAR
approved these changes
Mar 6, 2018
BridgeAR
added
the
author ready
|
@daynin Do you have an opinion on whether this PR should land, given your benchmark results? If yes, this could land as it is, I think. |
|
@addaleax yeah, I think it should land if nobody has any objections |
addaleax
removed
the
author ready
addaleax
pushed a commit
that referenced
this pull request
Mar 23, 2018
PR-URL: #19076 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
addaleax
pushed a commit
that referenced
this pull request
Mar 23, 2018
PR-URL: #19076 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos
pushed a commit
that referenced
this pull request
Mar 30, 2018
PR-URL: #19076 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos
pushed a commit
that referenced
this pull request
Mar 30, 2018
PR-URL: #19076 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
url, querystring, internal/url, internal/querystring