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.

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

Ensure assertion message is a string - Fixes #1125 #1174

Conversation

leewaygroups
Copy link

This PR is intended to fix #1125 .

  1. The approach taken is to ensure/force assertion message to string.
  2. This is handled in a single helper function stringMsg and
  3. stringMsg is invoked only in assertion methods it is needed (in my opinion)

Note: If msg is an object and non Error instance, the msg object is stringified using JSON.stringify.

All comments/reviews are welcome.

Cheers!!

lib/assert.js Outdated
x.pass = msg => {
test(true, create(true, true, 'pass', msg, x.pass));
test(true, create(true, true, 'pass', stringMsg(msg), x.pass));
Copy link
Member

Choose a reason for hiding this comment

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

Instead of wrapping every msg, you can just add the validation to the create function.

Copy link
Author

Choose a reason for hiding this comment

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

@sindresorhus : Great catch! I wonder why I didn't see that.

lib/assert.js Outdated
@@ -30,45 +30,49 @@ function test(ok, opts) {
}
}

function stringMsg(msg) {
return msg instanceof Error ? msg.message : typeof msg === "string" ? msg : msg ? JSON.stringify(msg) : null;
Copy link
Member

Choose a reason for hiding this comment

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

It should fail if msg is anything other than a string, not gracefully handle it.

Copy link
Author

Choose a reason for hiding this comment

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

Following your comment, I can think of two possible ways to go about this.
Throw an exception when msg is anything other than

  1. A string or
  2. A string or Error object (since for errro type, the message propoerty value can be used).

Copy link
Member

Choose a reason for hiding this comment

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

  1. Message should always be a string

@sindresorhus sindresorhus changed the title Fix#1125 check assertion is a string Ensure assertion message is a string - Fixes #1125 Jan 9, 2017
lib/assert.js Outdated
stackStartFunction: fn
};
}
throw new TypeError('assertion message must be a string');
Copy link
Member

Choose a reason for hiding this comment

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

Would be better to invert the check and put the throw in the if-statement. Also: assertion => Assertion

Copy link
Author

Choose a reason for hiding this comment

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

I can check if msg exists and not string then throw an error in within the if-statement:

if(msg && typeof msg !== 'string'){ throw error }

But that logic is flawed because 0 will slip through uncaught. Also, inverting the logic as suggested seem flawed as well since an OR condition is short-circuited once first test is truthy.

I'll look more closely.

Cheers!

Copy link
Member

Choose a reason for hiding this comment

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

I simply meant:

if (msg !== undefined && typeof msg !== 'string') {
	throw ...
}

return ...

@sindresorhus
Copy link
Member

This looks good now, but needs a test.

Copy link
Member

@novemberborn novemberborn left a comment

Choose a reason for hiding this comment

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

("Reviewing" so the PR status reflects that this still needs a test. No worries if you're busy @leewaygroups.)

@leewaygroups
Copy link
Author

@novemberborn Yeah, some delays from me there. Sorry about that. I'll check-in the test in shortly.

@novemberborn
Copy link
Member

Hi @leewaygroups, just checking in 😄

@sindresorhus
Copy link
Member

Closing for lack of activity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants