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

Adding unary operator + to transform string into numbers #10

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Adding unary operator + to transform string into numbers #10

wants to merge 1 commit into from

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