JSDoc for Enumeration and Enumeration values

[jessegreenberg]

There was some discussion over slack and we decided to briefly talk about with the team. What is the correct JSDoc to annotate Enumerations and Enumeration values? For an enumeration, is the following correct?

    // @public {Enumeration}
    this.sceneModeChoice = new Enumeration( [ 'SAME_LENGTH', 'ADJUSTABLE_LENGTH' ] );

When writing JSDoc for a value of MyEnumeration, is the following correct?

  // @param {*} - a value of MyEnumeration

[samreid]

We discussed conventions in #53 and documented them in https://github.com/phetsims/phet-core/blob/master/js/Enumeration.js.

For masses-and-springs/js/intro/model/IntroModel.js, it seems incorrect to assign enumerations as instance properties. It seems they would be better as statics or static properties.

Here's a patch that demonstrates how to apply these principles to IntroModel (for one of the Enums):

Index: js/intro/model/IntroModel.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/intro/model/IntroModel.js	(revision fae00b4fefdccfc9d871e696a8a9c146ed23607c)
+++ js/intro/model/IntroModel.js	(date 1561505848000)
@@ -19,6 +19,9 @@
   var PropertyIO = require( 'AXON/PropertyIO' );
   var StringIO = require( 'TANDEM/types/StringIO' );
 
+  // constants
+  const LengthTypeEnum = new Enumeration( [ 'SAME_LENGTH', 'ADJUSTABLE_LENGTH' ] );
+
   /**
    * @param {Tandem} tandem
    *
@@ -30,20 +33,17 @@
     var self = this;
     this.basicsVersion = false;
 
-    // @public {Enumeration}
-    this.sceneModeChoice = new Enumeration( [ 'SAME_LENGTH', 'ADJUSTABLE_LENGTH' ] );
-
     // @public {Enumeration} Choices for constant parameter
     this.constantModeChoice = new Enumeration( [ 'SPRING_CONSTANT', 'SPRING_THICKNESS' ] );
 
     this.addDefaultSprings( tandem );
     this.addDefaultMasses( tandem );
 
-    // @public {Property.<string>} determines the scene selection for the intro screen
-    this.sceneModeProperty = new Property( this.sceneModeChoice.SAME_LENGTH, {
+    // @public {Property.<LengthTypeEnum>} determines the scene selection for the intro screen
+    this.sceneModeProperty = new Property( LengthTypeEnum.SAME_LENGTH, {
       tandem: tandem.createTandem( 'sceneModeProperty' ),
       phetioType: PropertyIO( StringIO ),
-      validValues: [ this.sceneModeChoice.SAME_LENGTH, this.sceneModeChoice.ADJUSTABLE_LENGTH ]
+      validValues: LengthTypeEnum.VALUES
     } );
 
     // @public {Property.<string|null>} determines which spring property to keep constant in the constants panel
@@ -60,7 +60,7 @@
     // We are updating the spring thickness for each spring, whenever we are on the first scene
     this.springs.forEach( function( spring ) {
       spring.springConstantProperty.link( function( springConstant ) {
-        if ( self.sceneModeProperty.get() === self.sceneModeChoice.SAME_LENGTH ) {
+        if ( self.sceneModeProperty.get() === LengthTypeEnum.SAME_LENGTH ) {
           spring.updateThickness( spring.naturalRestingLengthProperty.get(), springConstant );
         }
       } );
@@ -77,7 +77,7 @@
 
     // Link that is responsible for switching the scenes
     this.sceneModeProperty.lazyLink( function( scene ) {
-      if ( scene === self.sceneModeChoice.SAME_LENGTH ) {
+      if ( scene === LengthTypeEnum.SAME_LENGTH ) {
 
         // Manages stashing and applying parameters to each scene
         self.resetScene( true );
@@ -91,7 +91,7 @@
         self.setSpringState( sameLengthModeSpringState );
       }
 
-      else if ( scene === self.sceneModeChoice.ADJUSTABLE_LENGTH ) {
+      else if ( scene === LengthTypeEnum.ADJUSTABLE_LENGTH ) {
 
         // Manages stashing and applying parameters to each scene
         self.resetScene( true );
@@ -123,7 +123,7 @@
     Property.multilink( [ this.constantParameterProperty, this.sceneModeProperty ], function( selectedConstant, scene ) {
 
       // Only adjust thickness/springConstant on adjustableLength scene
-      if ( scene === self.sceneModeChoice.ADJUSTABLE_LENGTH ) {
+      if ( scene === LengthTypeEnum.ADJUSTABLE_LENGTH ) {
 
         // Manages logic for changing between constant parameters
         if ( selectedConstant === self.constantModeChoice.SPRING_CONSTANT ) {
@@ -211,14 +211,13 @@
      * @private
      */
     initializeScenes: function() {
-      this.sceneModeProperty.set( this.sceneModeChoice.ADJUSTABLE_LENGTH );
+      this.sceneModeProperty.set( LengthTypeEnum.ADJUSTABLE_LENGTH );
       this.resetScene( false );
       this.spring1.naturalRestingLengthProperty.set( 0.25 );
 
       // initial parameters set for both scenes
-      this.sceneModeProperty.set( this.sceneModeChoice.SAME_LENGTH );
+      this.sceneModeProperty.set( LengthTypeEnum.SAME_LENGTH );
       this.resetScene( false );
     }
   } );
-} )
-;
+} );

[jessegreenberg]

Thanks @samreid, that looks good. The only difference between this and #53 is that it was decided that Enumerations don't need to identify that they are enumerations in the name like LengthType instead of LengthTypeEnum. There was some talk about this on slack recently too, should we re-assess this?

Following with the above example, lets say we have a function setLengthMode that takes one of the values of LengthTypeEnum. Is this the correct JSDoc?

/**
 * Sets the scene.
 * @param {*} mode - value from LengthTypeEnum
 */
setSceneMode( mode ) {
  assert && assert( LengthTypeEnum.includes( mode ) );
  //...
}

[samreid]

I agree we decided Enums don't need to end with *Enum suffix. For the JSDoc, I think we are treating the enums like types and we are treating the enum values as instances of those types, so it would be documented like this:

/**
 * Sets the scene.
 * @param {LengthTypeEnum} mode - value from LengthTypeEnum
 */
setSceneMode( mode ) {
  assert && assert( LengthTypeEnum.includes( mode ) );
  //...
}
``

[jessegreenberg]

I think we are treating the enums like types and we are treating the enum values as instances of those types

Awesome, thanks! This confused me, I thought it was odd to think of Enumeration values as instances of the Enumeration. But I noticed this is how Java does things too.

[jessegreenberg]

I added point (6) about this under the other "Considerations for using Enumeration"

/**
 * (6) Values of the Enumeration are considered instances of the Enumeration in documentation. For example, a method
 *     that that takes an Enumeration value as an argument would be documented like this:
 *
 *     // @param {Scene} mode - value from Scene Enumeration
 *     setSceneMode( mode ) {
 *       assert && assert( Scene.includes( mode ) );
 *       //...
 *     }
 */

[zepumph]

We discussed this today in the developer meeting, and the consensus that @samreid and @jessegreenberg outlined above was spot on.

I updated the code review checklist, and created phetsims/masses-and-springs#349. I think this is ready to close.