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.

Sign up for GitHub

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

Jump to bottom

errors: refactor invalidArgType() #15544

Closed
wants to merge 1 commit into from
Closed

errors: refactor invalidArgType() #15544

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/internal/errors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 ')) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

determiner = 'must not be';
expected = expected.split('not ')[1];
} else {
Expand All @@ -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')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason not to check the type here too?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@refack refack Sep 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I'm also -0 on over guarding an internal function.
IMHO if at all it should be at asserted around L309:

node/lib/internal/errors.js

Lines 307 to 309 in adbed02

function invalidArgType(name, expected, actual) {
internalAssert(name, 'name is required');

// for the case like 'first argument'
msg = `The ${name} ${determiner} ${oneOf(expected, 'type')}`;
} else {
Expand Down