phetsims / tandem

Simulation-side code for PhET-iO
MIT License
0 stars 5 forks source link

Get the tandem used for string Properties #298

Closed pixelzoom closed 1 year ago

pixelzoom commented 1 year ago

In https://github.com/phetsims/fourier-making-waves/issues/235#issuecomment-1577122109, we’re proposing to add a Tandem under fourierMakingWaves.general.model.strings.fourierMakingWaves. That Tandem appears to be created by getStringsModule.ts (line 197). Is there a way to get that Tandem from within sim-specific code, and not duplicate it in an adhoc manner?

By adhoc, I mean something like this:

const DERIVED_STRINGS_TANDEM = Tandem.GENERAL_MODEL.createTandem( 'strings' ).createTandem( 'fourierMakingWaves' ).createTandem( 'derivedStrings' );

… which will break if strings are every relocated.

pixelzoom commented 1 year ago

From Slack#phet-io:

Sam Reid Maybe add a new function to Tandem? Like getStringTandem(requireJSNamespace)?

Michael Kauzmann Or really just Tandem.MODEL_STRINGS

Michael Kauzmann I think I like SR's better

zepumph commented 1 year ago

console.log( Tandem.getStringsTandem().phetioID );

fourierMakingWaves.general.model.strings.fourierMakingWaves.derivedStrings

console.log( Tandem.getStringsTandem( 'otherRepo' ).phetioID );

fourierMakingWaves.general.model.strings.otherRepo.derivedStrings

I updated your usage in FMW. Do you want to use this directly instead of the factored out constant? Feel free to close.

pixelzoom commented 1 year ago

Tandem.getStringsTandem is incorrect. It's not returning the Tandem to strings for the module. It's incorrectly tacking on "derivedStrings".

  /**
   * Get the Tandem location for custom model strings. Provide the camelCased repo name for where the string should be
   * organized. This will default to the sim's name.
   */
  public static getStringsTandem( moduleName: string = Tandem.ROOT.name ): Tandem {
    return Tandem.GENERAL_MODEL.createTandem( 'strings' ).createTandem( moduleName ).createTandem( 'derivedStrings' );
  }
pixelzoom commented 1 year ago

If you intended Tandem.getStringsTandem to return a place for putting derived strings, let's name it accordingly. But imo we should have Tandem.getStringsTandem that gets the Tandem for strings, and Tande.getDerivedStringsTandem to standardize the convention for where to add derived strings.

pixelzoom commented 1 year ago

In the above commit, Tandem now has getStringsTandem and getDerivedStringsTandem. We can rename getDerivedStringsTandem when we resolve the design issue of where these strings should live in the Studio tree. But for now, they live under 'derivesStrings'.

@zepumph please review. Close if OK.

zepumph commented 1 year ago

I'm sorry about that, I definitely didn't understand fully, and I believe that you will also see that in how I spoke in https://github.com/phetsims/fourier-making-waves/issues/235#issuecomment-1583007493. I like your commits, but I could also see us getting rid of getDerivedStringsTandem. Do you have a preference?

pixelzoom commented 1 year ago

I would like to resolve the design issue of where to put derived string Properties. I suspect (and @arouinfar has so far agreed) that this should be standardized. And if that proves to be the case, then we should have getDerivedStringsTandem to handle the standardization. Having only getStringsTandem will result in these strings ending up in different places in the Studio tree, under different tandem names.

pixelzoom commented 1 year ago

So.... Let's leave this issue open until the design questions raised in https://github.com/phetsims/fourier-making-waves/issues/235 are resolved. I'll leave it assigned to me, "on hold".

pixelzoom commented 1 year ago

In https://github.com/phetsims/fourier-making-waves/issues/235#issuecomment-1588032885, we decided to put derived string Properties under a "derived" tandem name. So we will keep both getDerivedStringsTandem and getStringTandem.

Closing.