Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should tandem be in options or required param? #489

Closed
zepumph opened this issue Apr 26, 2018 · 5 comments
Closed

Should tandem be in options or required param? #489

zepumph opened this issue Apr 26, 2018 · 5 comments

Comments

@zepumph
Copy link
Member

zepumph commented Apr 26, 2018

For joist, @samreid and I have implemented tandems as required parameters. @pixelzoom in #487 pointed out that he wished that they were in options. I'm not just I see the benefit in moving these to options in joist. @pixelzoom what were your concerns?

@zepumph zepumph changed the title Should tandem be in options or required param Should tandem be in options or required param? Apr 26, 2018
@pixelzoom
Copy link
Contributor

pixelzoom commented Apr 27, 2018

Three concerns:

(1) Having tandem as a required parameter complicates adding components to the joist demo application. Unless we're planning to instrument the demo application, that means that I have to pass Tandem.optional (I think) into any joist component.

(2) In general, anything that has a reasonable default is typically passed via options. tandem certainly has a reasonable default, so I see no benefit to doing something different in joist. Which brings me to...

(3) One way of providing tandem is preferable to the current two ways. Less to think about, less to get wrong, joist components work like all other components, etc.

@pixelzoom pixelzoom assigned zepumph and unassigned pixelzoom Apr 27, 2018
@pixelzoom
Copy link
Contributor

(4) Having tandem as a required parameter makes it unnecessarily complicated to test in the browser console. E.g. the case I ran into while debugging/testing yesterday:

new phet.joist.KeyboardHelpDialog( ...., phet.tandem.Tandem.optional ).show()

vs.

new phet.joist.KeyboardHelpDialog( .... ).show()

All code should be testable without tandems.

@pixelzoom
Copy link
Contributor

I've changed my opinion since #489 (comment). I tried a few different approaches when instrumenting Hooke's Law. The approach that worked the best for instrumenting sim-specific code was to:

(1) Use @param tandem for constructors that didn't have an options parameter. This typically included top-level model and view types that are specific to the sim.

(2) Use option tandem: Tandem.required for constructors that already have an options parameter. This default was super helpful for identify cases where I had neglected to pass a tandem.

I haven't instrumented any common code yet. But I suspect the tandem is best passed via options, with default tandem. Tandem.optional.

@samreid
Copy link
Member

samreid commented Jun 15, 2018

The description in #489 (comment) sounds great to me, should we capture it in the "how to instrument" documentation and close this issue?

@zepumph
Copy link
Member Author

zepumph commented Aug 21, 2018

will be handled as part of https://github.com/phetsims/phet-io/issues/1243. Closing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants