Skip to content

Conversation

@mgreter
Copy link
Contributor

@mgreter mgreter commented Mar 14, 2018

No description provided.

@mgreter mgreter force-pushed the arithmetic-edge-cases branch from e6d69ec to 7af3876 Compare March 14, 2018 16:26
@mgreter mgreter merged commit 23d8b23 into sass:master Mar 14, 2018
Copy link
Contributor

@nex3 nex3 left a comment

Choose a reason for hiding this comment

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

I don't think the specced behavior for mod 0 is correct here. It doesn't make sense to me that division by zero would be allowed but modulo would not. I think mod 0 should produce NaN.

s: (5em / 2);
t: 1 + (2 + (3/4 + (4/5 6/7)));
u: (1 / 0); // Infinity
v: (0 / 0); // NaN
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd really like to move to more self-descriptive test cases, rather than ones that are labeled arbitrarily using numbers or letters. For example, these could be written

  one-over-zero: (1 / 0);
  zero-over-zero: (0 / 0);

This makes it easier to tell what the test cases is when looking up the expected output, and in more complex cases to determine what the point of the test is.

t: 1 + (2 + (3/4 + (4/5 6/7)));
u: (1 / 0); // Infinity
v: (0 / 0); // NaN
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why these test cases were added to a directory called "arithmetic and lists" when they don't seem to have anything to do with lists.

nex3 added a commit that referenced this pull request Mar 15, 2018
To match the behavior of dividing by zero, this should produce NaN
rather than failing.

See also #1228
@nex3 nex3 mentioned this pull request Mar 15, 2018
nex3 added a commit that referenced this pull request Mar 16, 2018
To match the behavior of dividing by zero, this should produce NaN
rather than failing.

See also #1228
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.

2 participants