util: extract out encoding validation functions#18421
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot added the encoding 
timotew changed the title timotew changed the title 
cjihrig left a comment
cjihrig left a comment There was a problem hiding this comment.
If we go this route, I think validateEnDecoder() should be split into two separate functions.
jasnell commented Jan 29, 2018
+1 to splitting out |
BridgeAR left a comment
BridgeAR left a comment There was a problem hiding this comment.
Please do not change the validation or the error messages. Otherwise LG
lib/internal/encoding.js Outdated
| } | ||
| functionvalidateArgument(prop,expected,propName){ | ||
| if(typeofprop!==expected&&!isArrayBufferView(prop)) |
There was a problem hiding this comment.
This is not only a extraction but it changes the validation itself as it also validates that the input is not an ArrayBufferView. I would like to keep it as is.
lib/internal/encoding.js Outdated
| encoding=`${encoding}`; | ||
| if(typeofoptions!=='object') | ||
| thrownewerrors.Error('ERR_INVALID_ARG_TYPE','options','Object'); | ||
| validateArgument(options,'object','options'); |
There was a problem hiding this comment.
By combining the check for the type with the second ERR_INVALID_ARG_TYPE argument the error message gets changed. That should not be the case. That applies to all validateArgument calls.
There was a problem hiding this comment.
@BridgeAR the string type checks error messages is not affected.
lib/internal/encoding.js Outdated
| } | ||
| functionvalidateArgument(prop,expected,propName){ | ||
| if(typeofprop!==expected.toLowerCase()) |
There was a problem hiding this comment.
Using toLowerCase is a performance overhead.
I suggest to either split the validation in two functions or to just keep these validations as they were. You can of course also add a fourth argument.
There was a problem hiding this comment.
@BridgeAR is this last commit inline with your suggestion?
lib/internal/encoding.js Outdated
| if(isArrayBuffer(input)){ | ||
| validateArgumentBuffer(input,'input'); | ||
| if(isArrayBuffer(input)) |
There was a problem hiding this comment.
This results in a second isArrayBuffer check and is definitely not desirable. Please keep that check as it was before (as in: remove validateArgumentBuffer again).
BridgeAR added the author ready 
BridgeAR commented Jan 31, 2018
PR-URL: nodejs#18421 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
BridgeAR commented Feb 1, 2018
Landed in b0b2045 I fixed the commit message while landing (adding the subsystem & shortening it) |
addaleax removed the author ready 
PR-URL: #18421 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: #18421 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: #18421 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins commented Mar 20, 2018
Should this be backported to |
PR-URL: nodejs#18421 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
7 participants
I saw some duplicate validations within
internal/encoding.js, so I extracted them out to separate functions.Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
util