tools.parseUnitString and tools.parseAngle

[iamdustan]

Building upon these previous conversations, here are some converting methods to start accepting angles in any format throughout the project.

#142 (comment)
#165 (comment)

+@basecode

[basecode]

I think parseFloat works quite well without the regex.

[iamdustan]

haha. very good point. It was early when I did that, I swear.

[basecode]

I like the idea of supporting units. tools.parseAngle sounds good. Not sure if we need tools.parseUnitString though.

[iamdustan]

parseUnitString was meant to be more low level. Currently it’s motivated to be the string-to-parts work of parseAngle and letting parseAngle really just deal with the conversion, but other unit types have different units of measure. E.g. 1s 1000ms 3min 50%

[basecode]

That makes sense.

[basecode]

parseAngle should also allow numbers (that are handled as radians). That would allow us to parse the parameter independent from the type. var radians = tools.parseAngle(param);

[iamdustan]

Good catch.

Simple enough as if (typeof angle === 'number') return angle; or should there be more robust handling of sorts (not sure what that would be...)

[basecode]

I'm tempted to let parseUnitString only return the unit and not "amount". For three reasons:

1. parseFloat is nothing we need to put in a helper function. It is already a well tested helper function itself and can safely be used.
2. We don't need to allocate another object (to be able to return 2 values)
3. It saves use from using if-else

Code:

extractUnit: function (any) {
  // make sure it's a string and remove trailing whitespace
  var unit = String(any).replace(/\s+$/, '');
  // returns extracted unit or empty string
  return unit.match(/[a-z]*$/i)[0];
}

parseAngle: function(angle) {

  var radians;
  var amount = parseFloat(angle);
  var unit = extractUnit(angle);

  switch(unit) {
    case 'deg': 
      radians = amount * PI / 180;
      break;
    …
    case '': // default
    case 'rad':
      radians = amount;
      break;
  }

  return radians;

}

What do you think?

[iamdustan]

👍

I don’t know why I had completely blanked and not thought of 1. I also disliked it very much for number two so that is a big one. Should we handle someone having white space between the number and unit as well?

'7 deg'?

[basecode]

extractUnit("7 deg") // "deg"
That's fine, isn't it? Your regex does already the work of ignoring leading whitespace.

[iamdustan]

👍 Any other cases we can think of to test and handle?

[basecode]

Please put radians at the beginning. Variables that are not explicitly assigned should be listed first. I know there aren't any coding-styles available yet for bonsai.. but soon :)

[iamdustan]

......annnnnnnnd done. I hadn’t even realized that nuance in the bonsai style. I’ve been trying to keep my additions like they belong in the family :)

[basecode]

Since parseUnitString can not just parse strings but also arrays (parseUnitString(["7em"])) for example wouldn't it be better to rename parseUnitString to extractUnit and change the JSDOC's "@ param" to {any}?

[iamdustan]

Yup. That does make sense. I didn’t even think of the example of passing an array in. Update coming.

[basecode]

I don't think that we need that check. What about "", null, [], {}, …? I see your point but I don't think that we should expect undefined and number of all types. The cast in L357 takes care of invalid values.

[iamdustan]

so do some test after var unit = ... to make sure we return the empty string if given a poor argument?

String(undefined) //=> "undefined"
String(null) //=> "null"
String({}) //=> "[object Object]"
String([]) //=> ''
String(['singleDepth']) //=> 'singleDepth'
String(['bigger', 'array', 4]) //=> 'bigger,array,4'

[basecode]

No. That's sth the author/dev need to fix. That's why I suggested to remove the type-checks.

[iamdustan]

👍 I can roll with that. Removed the checks, tests are still passing.

[basecode]

Oh and please add a line to the changelog :)

[basecode]

And thanks for your endurance! This is going to be a really nice feature!

[iamdustan]

The endurance is easy over here! I built it up working on matrices, getters/setters, and compounding transformations that I only half understand.

[iamdustan]

Pushed.

[basecode]

Thanks for this addition @iamdustan ! 👍

[iamdustan]

:) We made it!

[klipstein]

Awesome!