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

added check for logging negative zero #298

Merged
merged 4 commits into from Nov 3, 2014

Conversation

@dasilvacontin
Copy link
Contributor

@dasilvacontin dasilvacontin commented Oct 31, 2014

As @Lexicality mentions in issue #223, when asserting 0 against -0, Chai throws AssertionError: expected 0 to deeply equal 0 without any further information.

With the check in this PR, Chai throws AssertionError: expected 0 to deeply equal -0.

@@ -191,6 +191,9 @@ function formatPrimitive(ctx, value) {
return ctx.stylize(simple, 'string');

case 'number':
if (value === 0 && Infinity !== (1 / value)) {

This comment has been minimized.

@keithamus

keithamus Oct 31, 2014
Member

Nice work @dasilvacontin but there is some serious reverse logic going on here. Surely this would be better written as value === 0 && (1/value) === -Infinity?

This comment has been minimized.

@dasilvacontin

dasilvacontin Oct 31, 2014
Author Contributor

I totally agree with that! Thanks for pointing it out, @keithamus ! 😄

@keithamus
Copy link
Member

@keithamus keithamus commented Nov 1, 2014

LGTM. @logicalparadox am I good to merge?

@logicalparadox
Copy link
Member

@logicalparadox logicalparadox commented Nov 1, 2014

Missing test asserting this works.

@Lexicality
Copy link

@Lexicality Lexicality commented Nov 1, 2014

👍

@dasilvacontin
Copy link
Contributor Author

@dasilvacontin dasilvacontin commented Nov 1, 2014

Added some assertions. If it needs any other change, suggestions are welcome.

@logicalparadox
Copy link
Member

@logicalparadox logicalparadox commented Nov 2, 2014

👍

keithamus added a commit that referenced this pull request Nov 3, 2014
added check for logging negative zero
@keithamus keithamus merged commit 3e35adf into chaijs:master Nov 3, 2014
1 check passed
1 check passed
@logicalparadox
continuous-integration/travis-ci The Travis CI build passed
Details
@keithamus
Copy link
Member

@keithamus keithamus commented Nov 3, 2014

Sorry @logicalparadox about missing the test - I'll ensure to be extra vigilant next time!

@dasilvacontin
Copy link
Contributor Author

@dasilvacontin dasilvacontin commented Nov 3, 2014

Sorry for missing it too! :/

@dasilvacontin dasilvacontin deleted the dasilvacontin:negativeZeroLogging branch Nov 27, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants