Add support for ":" assignment to tandem-name-should-match

Question

Add support for ":" assignment to tandem-name-should-match

Closed this issue 7 months ago · 7 comments

Over in phetsims/faradays-electromagnetic-lab#24 (comment), the tandem-name-should-match rule did not catch this copy-paste error in FELPreferences.ts:

  magneticUnitsProperty: new StringUnionProperty<MagneticUnits>( FELQueryParameters.magneticUnits as MagneticUnits, {
    validValues: MagneticUnitsValues,
-   tandem: Tandem.PREFERENCES.createTandem( 'functionVariableProperty' ),
+   tandem: Tandem.PREFERENCES.createTandem( 'magneticUnitsProperty' ),
    phetioFeatured: true
  } ),

It would nice if tandem-name-should-match supported this form of assignment.

Answer 1 · 2024-01-12T02:49:49.000Z

Yes, I see a few occurrences in that file where they don't match. However, I see other cases like https://github.com/phetsims/geometric-optics/blob/cf7d79e0650aad9d1e66c20d2f67e1f0fdd6c2fe/js/common/view/VisibilityCheckboxGroup.ts#L87-L101 where it is apparently not supposed to match. Since we know there may be cases where it is not supposed to match, is this a good candidate for a lint rule?

Answer 2 · 2024-01-15T16:22:10.000Z

https://github.com/phetsims/geometric-optics/blob/cf7d79e0650aad9d1e66c20d2f67e1f0fdd6c2fe/js/common/view/VisibilityCheckboxGroup.ts#L87-L101 is an example of where the "group" API (in this case VerticalCheckboxGroup) is failing us. PhET-iO element virtualImageCheckboxEnabledProperty should actually be virtualImageCheckbox.enabledProperty, but that's awkward/brittle to finagle via the Items API. So in this example, we would use // es-lint-disable-line tandem-name-should-match to opt out.

Answer 3 · 2024-01-16T04:35:04.000Z

I created this working patch:

Subject: [PATCH] Show histogram open by default
---
Index: eslint/rules/tandem-name-should-match.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/eslint/rules/tandem-name-should-match.js b/eslint/rules/tandem-name-should-match.js
--- a/eslint/rules/tandem-name-should-match.js	(revision 3b8cb80a85beb450eb2667a8ebaeb1eb8209f9c7)
+++ b/eslint/rules/tandem-name-should-match.js	(date 1705379385789)
@@ -76,6 +76,22 @@
             }
           }
         }
+      },
+
+      Property( node ) {
+        if ( node.key.type === 'Identifier' ) {
+          const keyName = node.key.name;
+
+          // Assuming a similar function to check the naming convention
+          const tandemName = getCreateTandemCallArgument( node.value );
+
+          if ( tandemName && !matchesNamingConvention( tandemName, keyName ) ) {
+            context.report( {
+              node: node,
+              message: `The property key "${keyName}" does not match the argument passed to createTandem "${tandemName}"`
+            } );
+          }
+        }
       }
     };
   }
@@ -131,6 +147,7 @@
       );
 
       if ( tandemProperty ) {
+
         const createTandemCall = tandemProperty.value;
 
         if (
@@ -146,7 +163,18 @@
 
           // QUIRK: If the tandem is something like: myAccordionBoxTandem.createTandem('theChild') then the const variable may have
           // a name like myAccordionBoxChild, so we need to remove the 'Tandem' part from the name before checking.
-          if ( callee === 'tandem' || callee === 'options.tandem' || callee === 'providedOptions.tandem' ) {
+
+          // TODO: Should we add it for any of these keys? See https://github.com/phetsims/chipper/issues/1416
+          // Tandem.ROOT
+          // Tandem.ROOT_TEST
+          // Tandem.GENERAL_MODEL
+          // Tandem.GENERAL_VIEW
+          // Tandem.GENERAL_CONTROLLER
+          // Tandem.GLOBAL_MODEL
+          // Tandem.GLOBAL_VIEW
+          // Tandem.COLORS
+          // Tandem.STRINGS
+          if ( callee === 'tandem' || callee === 'options.tandem' || callee === 'providedOptions.tandem' || callee === 'Tandem.PREFERENCES' ) {
 
             const argument = createTandemCall.arguments[ 0 ];

Running grunt lint-everything --disable-eslint-cache --chip-away, I get this result:

Results from chipAway:

build-an-atom: @jbphet 3 errors in 2 files.
calculus-grapher: @veillette, @pixelzoom 1 errors in 1 files.
energy-skate-park: @jessegreenberg 1 errors in 1 files.
gas-properties: @pixelzoom 1 errors in 1 files.
geometric-optics: @pixelzoom 1 errors in 1 files.
masses-and-springs: @matthew-blackman 1 errors in 1 files.


All results (repeated from above)



/Users/samreid/phet/root/build-an-atom/js/atom/view/AtomScreenView.js
   79:9  error  The property key "titleNode" does not match the argument passed to createTandem "netChargeAccordionBoxTitleText"   tandem-name-should-match
  111:9  error  The property key "titleNode" does not match the argument passed to createTandem "massNumberAccordionBoxTitleText"  tandem-name-should-match

/Users/samreid/phet/root/build-an-atom/js/symbol/view/SymbolScreenView.js
  39:7  error  The property key "titleNode" does not match the argument passed to createTandem "symbolAccordionBoxTitleText"  tandem-name-should-match

/Users/samreid/phet/root/calculus-grapher/js/common/model/CalculusGrapherPreferences.ts
  57:3  error  The property key "predictPreferenceEnabledProperty" does not match the argument passed to createTandem "predictFeatureEnabledProperty"  tandem-name-should-match

/Users/samreid/phet/root/energy-skate-park/js/common/view/EnergySkateParkScreenView.js
  225:7  error  The property key "content" does not match the argument passed to createTandem "restartSkaterText"  tandem-name-should-match

/Users/samreid/phet/root/gas-properties/js/energy/view/InjectionTemperatureControl.ts
  66:9  error  The property key "valuePattern" does not match the argument passed to createTandem "valuePatternStringProperty"  tandem-name-should-match

/Users/samreid/phet/root/geometric-optics/js/common/view/VisibilityCheckboxGroup.ts
  92:11  error  The property key "enabledProperty" does not match the argument passed to createTandem "virtualImageCheckboxEnabledProperty"  tandem-name-should-match

/Users/samreid/phet/root/masses-and-springs/js/common/view/StopperButtonNode.js
  29:7  error  The property key "content" does not match the argument passed to createTandem "stopSignNode"  tandem-name-should-match

✖ 8 problems (8 errors, 0 warnings)

Please note the check in the patch: callee === 'Tandem.PREFERENCES' is specifically added, since FEL instantiates preferences values like:

const FELPreferences = {

  // Magnetic units to be displayed
  magneticUnitsProperty: new StringUnionProperty<MagneticUnits>( FELQueryParameters.magneticUnits as MagneticUnits, {
    validValues: MagneticUnitsValues,
    tandem: Tandem.PREFERENCES.createTandem( 'magneticUnitsProperty' ),
    phetioFeatured: true
  } ),

The default checks for tandem or options.tandem or providedOptions.tandem wouldn't have caught it anyways.

@pixelzoom should we commit this? How should we handle the guards on the tandem prefix? Should I eslint-disable the failures listed above?

Answer 4 · 2024-01-17T00:02:47.000Z

@pixelzoom should we commit this?

Yes, I think so. All of the cases that it caught in my sim were legitimate errors.

How should we handle the guards on the tandem prefix?

I don't understand the question. If you're asking how to deal with the various static Tandem instances, like Tandem.PREFERENCES.createTandem... How about replacing callee === 'Tandem.PREFERENCES' with a regex that would match any of the static Tandem instances?

Should I eslint-disable the failures listed above?

I fixed my sims. They were all tandem naming errors that had not been previously detected.

I inspected the others, and they are all appropriate to add
// eslint-disable-next-line tandem-name-should-match.
I would have done this, but it can't be done until the patch is committed.

Answer 5 · 2024-01-17T20:08:15.000Z

In discussion with @pixelzoom we would like to add || callee.startsWith( 'Tandem.' ) to the guard. I'll do that next.

Answer 6 · 2024-01-18T13:22:54.000Z

Combining the patch with || callee.startsWith( 'Tandem.' ) yields these 32 errors.

/Users/samreid/phet/root/build-an-atom/js/atom/view/AtomScreenView.js
   79:9  error  The property key "titleNode" does not match the argument passed to createTandem "netChargeAccordionBoxTitleText"   tandem-name-should-match
  111:9  error  The property key "titleNode" does not match the argument passed to createTandem "massNumberAccordionBoxTitleText"  tandem-name-should-match

/Users/samreid/phet/root/build-an-atom/js/symbol/view/SymbolScreenView.js
  39:7  error  The property key "titleNode" does not match the argument passed to createTandem "symbolAccordionBoxTitleText"  tandem-name-should-match

/Users/samreid/phet/root/energy-skate-park/js/common/view/EnergySkateParkScreenView.js
  225:7  error  The property key "content" does not match the argument passed to createTandem "restartSkaterText"  tandem-name-should-match

/Users/samreid/phet/root/gravity-and-orbits/js/common/GravityAndOrbitsColors.ts
  14:3  error  The property key "foregroundProperty" does not match the argument passed to createTandem "foregroundColorProperty"     tandem-name-should-match
  22:3  error  The property key "backgroundProperty" does not match the argument passed to createTandem "backgroundColorProperty"     tandem-name-should-match
  28:3  error  The property key "bodyLabelIndicatorProperty" does not match the argument passed to createTandem "labelColorProperty"  tandem-name-should-match

/Users/samreid/phet/root/masses-and-springs/js/common/view/StopperButtonNode.js
  29:7  error  The property key "content" does not match the argument passed to createTandem "stopSignNode"  tandem-name-should-match

/Users/samreid/phet/root/phet-io-wrappers/common/js/migration/MigrationReportTests.ts
  133:7  error  The variable name "phetioGroup" does not match the argument passed to createTandem "dogGroup"  tandem-name-should-match

/Users/samreid/phet/root/phet-io-wrappers/common/js/migration/processors/value-change-processors/ChangePhetioElementIOTypeTests.ts
  49:7  error  The variable name "oldProperty" does not match the argument passed to createTandem "myRangeProperty"  tandem-name-should-match

/Users/samreid/phet/root/phet-io/js/autoselectTests.ts
   93:3  error  The variable name "rectangle" does not match the argument passed to createTandem "phetioMouseHitRectangle1"                  tandem-name-should-match
  122:3  error  The variable name "rectangle" does not match the argument passed to createTandem "phetioMouseHitRectangle2"                  tandem-name-should-match
  149:3  error  The variable name "parent" does not match the argument passed to createTandem "parentNode1"                                  tandem-name-should-match
  228:3  error  The variable name "myStringProperty" does not match the argument passed to createTandem "phetioMouseHitView2StringProperty"  tandem-name-should-match
  232:3  error  The variable name "text" does not match the argument passed to createTandem "phetioMouseHitViewText"                         tandem-name-should-match
  251:3  error  The variable name "sibling" does not match the argument passed to createTandem "phetioMouseHitViewRectangle3"                tandem-name-should-match

/Users/samreid/phet/root/scenery-phet/js/accessibility/GrabDragInteractionTests.js
  41:3  error  The variable name "keyboardDragListener" does not match the argument passed to createTandem "myKeyboardDragListener"  tandem-name-should-match
  44:3  error  The variable name "interaction" does not match the argument passed to createTandem "myGrabDragInteraction"            tandem-name-should-match

/Users/samreid/phet/root/scenery/js/accessibility/globalKeyStateTracker.ts
  21:1  error  The variable name "globalKeyStateTracker" does not match the argument passed to createTandem "keyStateTracker"  tandem-name-should-match

/Users/samreid/phet/root/scenery/js/listeners/DragListenerTests.js
   24:5  error  The variable name "listener" does not match the argument passed to createTandem "myListener"  tandem-name-should-match
   42:5  error  The variable name "listener" does not match the argument passed to createTandem "myListener"  tandem-name-should-match
   61:5  error  The variable name "listener" does not match the argument passed to createTandem "myListener"  tandem-name-should-match
   84:5  error  The variable name "listener" does not match the argument passed to createTandem "myListener"  tandem-name-should-match
  110:5  error  The variable name "listener" does not match the argument passed to createTandem "myListener"  tandem-name-should-match
  135:5  error  The variable name "listener" does not match the argument passed to createTandem "myListener"  tandem-name-should-match

/Users/samreid/phet/root/scenery/js/listeners/FireListenerTests.js
  18:5  error  The variable name "listener" does not match the argument passed to createTandem "myListener"  tandem-name-should-match

/Users/samreid/phet/root/scenery/js/listeners/PressListenerTests.js
  20:5  error  The variable name "listener" does not match the argument passed to createTandem "myListener"  tandem-name-should-match
  96:5  error  The variable name "listener" does not match the argument passed to createTandem "myListener"  tandem-name-should-match

/Users/samreid/phet/root/sun/js/SliderTests.js
  21:3  error  The variable name "slider" does not match the argument passed to createTandem "mySlider"  tandem-name-should-match

/Users/samreid/phet/root/tandem/js/PhetioObjectTests.ts
  35:3  error  The variable name "obj" does not match the argument passed to createTandem "test1"  tandem-name-should-match
  47:3  error  The variable name "obj" does not match the argument passed to createTandem "test2"  tandem-name-should-match

/Users/samreid/phet/root/tandem/js/TandemTests.ts
  16:3  error  The variable name "property" does not match the argument passed to createTandem "aProperty"  tandem-name-should-match

✖ 32 problems (32 errors, 0 warnings)


Fatal error: Lint failed
Fatal error: perennial grunt "--repo=projectile-data-lab" "lint-everything" "--disable-eslint-cache" failed with code 1

I wasn't sure of the best way to proceed. Here are some options:

Omit || callee.startsWith( 'Tandem.' ) from the guard
Avoid this lint check in files that end with Tests.js or Tests.ts (a sizable fraction of the new errors)
eslint-disable the 32 errors.

@pixelzoom can you please advise?

Answer 7 · 2024-02-14T00:13:37.000Z

I'v changed my mind on this one. It's not worth the effort, not something I want to champion. Thanks for your work @samreid.

I don't see any changes that were committed to tandem-name-should-match.js, only proposed patches, so nothing to revert.

Closing.