Skip to content
This repository was archived by the owner on Feb 26, 2024. It is now read-only.

Conversation

@nicola
Copy link

@nicola nicola commented Mar 31, 2014

When reading the test files, I spot that parser.spec.js contained:

it('should parse unary - expressions', () => {
  expect(evaluate("-1")).toEqual(-1);
  expect(evaluate("+1")).toEqual(1);

So I divided them in two tests:

  1. should parse unary - expressions
  2. should parse unary + expressions

I realised that the unary + can be used to transform expressions of any type into numbers returning a number or NaN, see Mozilla Dev Network.

Therefore I implemented PrefixPlus similarly to PrefixNot. Here are the new tests that previously did not pass:

    it('should parse unary + expressions', () => {
      expect(evaluate("+1")).toEqual(1);
      expect(evaluate("+'1'")).toEqual(1);
      expect(evaluate("+'not a number'")).toEqual(NaN);
    });

However I am not sure if the angular team wants to add this level of complexity.

Review on Reviewable

@MikeMcElroy
Copy link

I'm uncomfortable with this -- I don't think it's a good idea for Expressionist to re-implement Javascript's funky type coercion you have in the second expectation, but I think there should be a test case to document what happens if someone tries to + or - a NaN-value. To that end, what should be the behavior around that circumstance?

@nicola
Copy link
Author

nicola commented Jul 26, 2014

I would personally expect to always have a javascript behavior in there.

@caitp
Copy link
Contributor

caitp commented Jul 26, 2014

expressions are more about data binding, the primary requirements of the expression parser are really just dereferencing properties of objects and calling functions (implemented in a controller somewhere). Negation, okay, but things like >>> 0 or +foo etc are a bit silly and out of scope for this, imo.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants