errors: refactor invalidArgType()
#15544
Conversation
`invalidArgType()` uses `includes()` in two places where `startsWith()` and `endsWith()` are more appropriate (at least in my opinion). Switch to those more specific functions.
Trott
added
the
errors
| @@ -320,7 +320,7 @@ function invalidArgType(name, expected, actual) { | |||
| if (Array.isArray(name)) { | |||
| var names = name.map((val) => `"${val}"`).join(', '); | |||
| msg = `The ${names} arguments ${determiner} ${oneOf(expected, 'type')}`; | |||
| } else if (name.includes(' argument')) { | |||
| } else if (name.endsWith(' argument')) { | |||
There was a problem hiding this comment.
Any reason not to check the type here too?
There was a problem hiding this comment.
It's checked in the if part. I mean, I guess it could be something other than a string or an array, but it's an internal function, so I'm not worried about end users sending an object or something.
My preference here is slight, though, and I can see the point of programming extra defensively here and adding the type check. If I do that, I'll want to add a test too to cover it.
There was a problem hiding this comment.
I'm also -0 on over guarding an internal function.
IMHO if at all it should be at asserted around L309:
Lines 307 to 309 in adbed02
| @@ -320,7 +320,7 @@ function invalidArgType(name, expected, actual) { | |||
| if (Array.isArray(name)) { | |||
| var names = name.map((val) => `"${val}"`).join(', '); | |||
| msg = `The ${names} arguments ${determiner} ${oneOf(expected, 'type')}`; | |||
| } else if (name.includes(' argument')) { | |||
| } else if (name.endsWith(' argument')) { | |||
There was a problem hiding this comment.
I'm also -0 on over guarding an internal function.
IMHO if at all it should be at asserted around L309:
Lines 307 to 309 in adbed02
| @@ -309,7 +309,7 @@ function invalidArgType(name, expected, actual) { | |||
|
|
|||
| // determiner: 'must be' or 'must not be' | |||
| let determiner; | |||
| if (expected.includes('not ')) { | |||
| if (typeof expected === 'string' && expected.startsWith('not ')) { | |||
There was a problem hiding this comment.
This should always be a string. A failure would be totally fine out of my perspective as we should have tests for this anyway.
But I think it is not really important for now.
|
Landed in cef6e1c |
`invalidArgType()` uses `includes()` in two places where `startsWith()` and `endsWith()` are more appropriate (at least in my opinion). Switch to those more specific functions. PR-URL: #15544 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Minwoo Jung <minwoo@nodesource.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
|
This does not land cleanly in v8.x. I believe it depends on a previously landed semver-major but not entirely sure. Would need a backport if it's relevant. |
`invalidArgType()` uses `includes()` in two places where `startsWith()` and `endsWith()` are more appropriate (at least in my opinion). Switch to those more specific functions. PR-URL: nodejs/node#15544 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Minwoo Jung <minwoo@nodesource.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
invalidArgType()usesincludes()in two places wherestartsWith()and
endsWith()are more appropriate (at least in my opinion). Switchto those more specific functions.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
errors