phetsims / phet-core

Core utilities used by all PhET simulations.
MIT License
8 stars 6 forks source link

Should we assert that Enumeration value texts are UPPERCASE? #44

Closed samreid closed 5 years ago

samreid commented 5 years ago

In https://github.com/phetsims/phet-core/issues/42#issue-371259394 @pixelzoom said:

In the examples and current usages, values are uppercase strings. Is that a convention? Should it be enforced? (There is currently no verification.)

samreid commented 5 years ago

@jonathanolson said:

This is what others had done, and was convention. Might be good to discuss at dev meeting (I'll tag the issue)

samreid commented 5 years ago

I said:

I had been using uppercase strings as I understood that to be the agreed-upon convention for our team. For example, in Wave Interference:

  const BarrierTypeEnum = {
    NO_BARRIER: 'NO_BARRIER',
    ONE_SLIT: 'ONE_SLIT',
    TWO_SLITS: 'TWO_SLITS'
  };
samreid commented 5 years ago

@pixelzoom said:

That sounds great, but should be enforced by Enumeration.

samreid commented 5 years ago

In the preceding commit, I added a regex test for each enumeration value and a unit test to make sure the regex test is working. @pixelzoom can you please review at your convenience? The regex test is:

      assert && keys.forEach(
        value => assert( value.match( /^[A-Z][A-Z0-9_]*$/g ),
          'Enumeration values should be uppercase alphanumeric with underscores and begin with a letter' )
      );
samreid commented 5 years ago

Here are some example tests with this regex:

'hello'.match( /^[A-Z][A-Z0-9_]*$/g )
null
'HELLO'.match( /^[A-Z][A-Z0-9_]*$/g )
["HELLO"]
''.match( /^[A-Z][A-Z0-9_]*$/g )
null
'_'.match( /^[A-Z][A-Z0-9_]*$/g )
null
'_X'.match( /^[A-Z][A-Z0-9_]*$/g )
null
'X'.match( /^[A-Z][A-Z0-9_]*$/g )
["X"]
'X_'.match( /^[A-Z][A-Z0-9_]*$/g )
["X_"]
'x'.match( /^[A-Z][A-Z0-9_]*$/g )
null
'DOG'.match( /^[A-Z][A-Z0-9_]*$/g )
["DOG"]
'DOG_7'.match( /^[A-Z][A-Z0-9_]*$/g )
["DOG_7"]
'DOG_or_CAT'.match( /^[A-Z][A-Z0-9_]*$/g )
null
pixelzoom commented 5 years ago

The test you have works fine, but returns truthy/falsy. I would've used /^[A-Z][A-Z0-9_]*$/g.test( value ), since it returns true/false. Your decision on whether the change it.

samreid commented 5 years ago

Thanks, I changed it to use regex.test. Back to you for microreview.

pixelzoom commented 5 years ago

👍