Conventions for specifying PhET-iO options

[pixelzoom]

For code review #41 ...

This is a question for the PhET-iO team, @zepumph and @samreid.

I've gotten in the habit of putting PhET-iO related option last at instantitation sites. That's the pattern that I've seen in most code, and I vaguely recall a conversation to that effect, so maybe it's an unwritten convention.

This sim puts that PhET-iO options wherever. For example, in MeanShareAndBalanceScreenView.ts:

   this.resetAllButton = new ResetAllButton( {
      listener: () => {
        this.interruptSubtreeInput(); // cancel interactions that may be in progress
        model.reset();
        this.reset();
      },
      tandem: options.tandem.createTandem( 'resetAllButton' ),
      right: this.layoutBounds.maxX - MeanShareAndBalanceConstants.SCREEN_VIEW_X_MARGIN,
      bottom: this.layoutBounds.maxY - MeanShareAndBalanceConstants.SCREEN_VIEW_Y_MARGIN
    } );

So... Does the PhET-iO team care when options like tandem appear? Or whether all PhET-iO options are grouped together? Or anything else that should be written into conventions?

[samreid]

and I vaguely recall a conversation to that effect, so maybe it's an unwritten convention.

I recall our decision to partition out the different options, and we sometimes include a // phet-io header for that section. But I feel this is somewhat less important with TypeScript since it is easy to see where each option is defined.

[marlitas]

I'm a little uncertain what to do with this issue... is there a convention I should be following? It makes sense to organize the options in a way that's consistent and legible, so I can move forward on that?

[pixelzoom]

I will defer to the PhET-iO team on this one. The convention has been to group all PhET-iO default under // phet-io, towards the end of the object literal. I don't fell strongly about whether this is necessary. I do think it's nice to group all of the PhET-iO defaults together, a11y defaults together, superclass defaults together, etc.

[pixelzoom]

... is there a convention I should be following? ...

Nothing in https://github.com/phetsims/phet-io/blob/master/doc/phet-io-instrumentation-technical-guide.md by the way, so if there's a convention, it's not documented.

[marlitas]

I will go ahead and move forward on grouping PhET-iO related options together at the end of the object literal. Seems to provide clarity, and I would probably recommend adding this as a convention to the phet-io instrumentation guide if this is something we want new devs to be aware of. @samreid and @zepumph... thoughts?

[zepumph]

https://github.com/phetsims/phet-info/blob/65750d4aee6583d968426e4edfee2bb570150f6e/doc/phet-software-design-patterns.md#L1110-L1112

This is where we documented it. Generally for all options grouping, not just for // phet-io. It did not get moved into the typescript conventions. I don't care at all about grouping them, I think it is up to new devs if it was a helpful thing to see when discovering new code (even though TypeScript shows where each option comes from). I'll do either (and will likely keep grouping options together even if we decide we don't put // {{group}} comments in anymore.

[pixelzoom]

In TypeScript code, I've found it more useful to group default values by the type that they are associated with. I generally put SelfOptions values first, followed by superclass values. For example, in geometric-optics LabelNode.ts:

type SelfOptions = {
  xAlign?: XAlign;
  yAlign?: YAlign;
  xOffset?: number; // from center, in view coordinates
  yOffset?: number; // in view coordinates
  phetioReadOnlyText?: boolean; // should the RichText be phetioReadOnly?
};

export type LabelNodeOptions = SelfOptions & PickRequired<BackgroundNodeOptions, 'visibleProperty' | 'tandem'>;

export default class LabelNode extends BackgroundNode {
  ...
  public constructor( ..., providedOptions: LabelNodeOptions ) {

    const options = optionize<LabelNodeOptions, SelfOptions, BackgroundNodeOptions>()( {

      // SelfOptions
      xAlign: 'center',
      yAlign: 'top',
      xOffset: 0,
      yOffset: 2,
      phetioReadOnlyText: false,

      // BackgroundNodeOptions
      xMargin: 5,
      yMargin: 5,
      rectangleOptions: {
        fill: GOColors.screenBackgroundColorProperty,
        cornerRadius: 4,
        opacity: 0.5
      }
    }, providedOptions );

[marlitas]

Categorized and organized options with phet-io. @pixelzoom can you review, and close if all looks good?

[pixelzoom]

👍🏻 closing