Clean up tandemNameSuffix and delete TANDEM_NAME_SUFFIX.

[pixelzoom]

For code review phetsims/projectile-data-lab#32 ...

Common-code things that have a required tandemName suffix provide TANDEM_NAME_SUFFIX. E.g. RectangularRadioButton.TANDEM_NAME_SUFFIX.

This sim has zero uses of TANDEM_NAME_SUFFIX. Lots of examples where it could be used, here are a few. Search for "tandemName: " and "createTandem" and you'll find more.

tandemName: 'springRadioButton',
tandemName: 'measuringTapeCheckbox'
tandemName: `mysteryLauncher${mysteryLauncher.launcherNumber}RadioButton`,
tandem: providedOptions.tandem.createTandem( 'launcherConfigurationProperty' ),
tandemName: 'field' + fieldNumber + 'RadioButton',

I'm on the fence about the importance of using TANDEM_NAME_SUFFIX to form tandem names, and whether that's actually even what TANDEM_NAME_SUFFIX should be used for. I know that I have not used it consistently, and I've never used ReadonlyProperty.TANDEM_NAME_SUFFIX. If we ever need to change a suffix, it will be less painful if we use TANDEM_NAME_SUFFIX. But the combined pain (and verbosity) of having to use TANDEM_NAME_SUFFIX is probably greater than the one-time pain of changing some suffix.

Anyway... You decide.

[samreid]

I'm on the fence about the importance of using TANDEM_NAME_SUFFIX to form tandem names, and whether that's actually even what TANDEM_NAME_SUFFIX should be used for.

Thanks for bringing this to my attention. I was unaware of cases where those suffixes are used to create the tandems, like:

https://github.com/phetsims/gas-properties/blob/257d84ff972ba6976a1bbeca106ec22a56ec9df1/js/common/view/ParticleTypeRadioButtonGroup.ts#L51-L62

I had been leveraging TANDEM_NAME_SUFFIX is as an assertion. For instance, to make sure that you don't accidentally call a Checkbox a RadioButton, and to ensure that instrumented Property instances have the term "Property" as a suffix. Adding string replacement like the above makes it difficult to search for tandems in the code which is otherwise very helpful and useful. For instance, if I see springRadioButton in studio, it is useful to be able to search for that same string in the codebase. I will check in with @matthew-blackman before we close or bring this back to @pixelzoom.

[pixelzoom]

I had been leveraging TANDEM_NAME_SUFFIX is as an assertion.

I agree, that is its main value, to provide tandemName standardization for PhET-iO.

Adding string replacement like the above makes it difficult to search for tandems in the code which is otherwise very helpful and useful.

Agreed there too -- that's a really valuable search that I do all the time.

So I think I'll avoid using TANDEM_NAME_SUFFIX to create tandem names, and would encourage you to continue to do the same for PDL.

That said... I'm wondering if we should make TANDEM_NAME_SUFFIX private, so that it's only used for verifying tandem names, not for creating tandem names. Thoughts on that?

[pixelzoom]

In the above commits, I replaced all usages of TANDEM_NAME_SUFFIX in my sims. They were 100% "RadioButton" and I was wondering why. Looking at Checkbox.ts, there is no TANDEM_NAME_SUFFIX, and instead there is tandemNameSuffix: 'Checkbox'. So another good reason to avoid TANDEM_NAME_SUFFIX is that it's not available consistently.

There are only 3 sim remaining uses of TANDEM_NAME_SUFFIX (1 in density-and-buoyancy-common, 2 in quadrilateral) so this may be a good time to make it private (quickest), or delete it and follow the tandemNameSuffix: pattern used in Checkbox.ts.

There are only 4 classes that define TANDEM_NAME_SUFFIX, and I've indicated the number of uses outside of the class:

• AquaRadioButton.TANDEM_NAME_SUFFIX (3 uses)
• ProfileColorProperty.TANDEM_NAME_SUFFIX (0 uses)
• ReadOnlyProperty.TANDEM_NAME_SUFFIX (0 uses)
• RectangularRadioButton.TANDEM_NAME_SUFFIX (0 uses)

[pixelzoom]

Yikes. In RectangularRadioButton.ts we have:

  public static readonly TANDEM_NAME_SUFFIX = 'RadioButton';
....
      tandemNameSuffix: 'Button',
...
 assert && assert( !options.tandem.supplied || options.tandem.name.endsWith( RectangularRadioButton.TANDEM_NAME_SUFFIX ),
      `RectangularRadioButton tandem.name must end with ${RectangularRadioButton.TANDEM_NAME_SUFFIX}: ${options.tandem.phetioID}` );

All of the above should be replaced with:

      tandemNameSuffix: 'RadioButton',

Almost as bad in AquaRadioButton:

  public static readonly TANDEM_NAME_SUFFIX = 'RadioButton';
...
      tandemNameSuffix: 'RadioButton',
...
    assert && assert( !options.tandem.supplied || options.tandem.name.endsWith( AquaRadioButton.TANDEM_NAME_SUFFIX ),
      `AquaRadioButton tandem.name must end with ${AquaRadioButton.TANDEM_NAME_SUFFIX}: ${options.tandem.phetioID}` );

[pixelzoom]

@samreid and I discussed. Most common-code is using tandemNameSuffix: 'something', and the TANDEM_NAME_SUFFIX cases are outliers. ComboBox uses an entirely different const name, ITEM_TANDEM_NAME_SUFFIX.

I'm going to take over this issue, transfer it to the tandem repo (the home of the tandemNameSuffix options), and retitle the issue.

To support ProfileColorProperty's arrow of valid suffixes, I'll investigated changing to tandemNameSuffix: string | string[].

I'll also look at cleaning up sun buttons, where tandemNameSuffix: 'Button' is repeated in many subclasses, and not present at all in the ButtonNode base class.

[pixelzoom]

This work is done. Option tandemNameSuffix is used throughout, and redundant suffix assertions were removed.

ReadOnlyProperty and Dialog were a little tricky, because they needed to handle archetypes:

// ReadOnlyProperty.ts
tandemNameSuffix: [ 'Property', DYNAMIC_ARCHETYPE_NAME ], // DYNAMIC_ARCHETYPE_NAME means that this Property is an archetype.

// Diaog.ts
tandemNameSuffix: [ 'Dialog', DYNAMIC_ARCHETYPE_NAME ], // DYNAMIC_ARCHETYPE_NAME means that this Dialog is an archetype.

There are only 2 places where I was unabled to get rid of TANDEM_NAME_SUFFIX:

(1) In ComboBox, we need const ITEM_TANDEM_NAME_SUFFIX = 'Item' because ComboBoxItem is a type, so suffix must be handled by ComboBox (not PhetioObject).

(2) Tandem.RootTandem.createTandem constrains its tandem names, and Tandem.SCREEN_TANDEM_NAME_SUFFIX is one of the allowed names. It would be preferrable if that constant were defined in Screen.ts, but tandem cannot depend on joist.

@samreid Would you like to have a look? Close if OK.

[samreid]

I reviewed the commits and the notes, and everything seems good to me. Nice work and thanks. Closing.