phetsims / tandem

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

Tandem name conventions are applied haphazardly. #267

Closed pixelzoom closed 2 years ago

pixelzoom commented 2 years ago

As discovered in https://github.com/phetsims/axon/issues/398 ...

In some cases, PhET wants to ensure that tandem names follows a specific naming convention. For example, all radio buttons should have a tandem name that ends with "RadioButton".

How those conventions are applied is implemented inconsistently, and in some cases not at all. Specifically:

So...

Is it desirable to add runtime checking of more tandem-name conventions? I think so. The alternative is to visually catch them in Studio, which is like looking for a needle in a haystack.

Should the implementation of those checks be similar? I think so. This will make adding additional checks easier, and make maintenance easier.

pixelzoom commented 2 years ago
  • tandem.name suffix is hardcoded in multiple places, instead of factored out (e.g. 'Property' in ReadOnlyProperty)

In the above commits, I factored out static TANDEM_NAME_SUFFIX where that suffix was being duplicated.

There are a few others that I identified (by searching for endsWith) but was unable to address:

arouinfar commented 2 years ago

Is it desirable to add runtime checking of more tandem-name conventions? I think so. The alternative is to visually catch them in Studio, which is like looking for a needle in a haystack.

I also think so. The less I need to manually review in Studio, the better.

samreid commented 2 years ago

I addressed the requests in the commits. There are still a few imperfections due to other constraints:

Ready for review.

UPDATE: I recalled this request:

Checkbox, Panel, AccordionBox, Dialog, "groups", most buttons, ...

Should this issue be a chip away for other occurrences too? Should we do that eagerly, or just as we are working in those areas?

pixelzoom commented 2 years ago
  • No convention is checked in places where there is a defacto standard: Checkbox, Panel, AccordionBox, Dialog, "groups", most buttons, ...

Yes, this should be discussed/addressed. Anything that has a design convention should have a corresponding check in the code to ensure that the convention is followed.

samreid commented 2 years ago

I reviewed the proposal with @marlitas and @pixelzoom and it sounded promising. The commits also have several TODOs pointing to this issue, some of which will need to move to side issues for help from the responsible developers.

samreid commented 2 years ago

All remaining work moved to side issues. @zepumph do you want to review this issue?

zepumph commented 2 years ago

I think a quick co-review would be the fastest here.

zepumph commented 2 years ago
zepumph commented 2 years ago

This blocks PhET-iO because tandem names will change for Text suffixing.

zepumph commented 2 years ago

In the next PhET-iO design meeting, we did a PSA about the new feature of PhET-iO elements called tandemSuffix. So if anyone finds a type, like Button, and not all tandem names end in "Button", we can add an assertion for that.

zepumph commented 2 years ago

renamed to tandemNameSuffix above (see https://github.com/phetsims/phet-io/issues/1855).

zepumph commented 2 years ago

@samreid, anything else here?

samreid commented 2 years ago

Everything looks great, nice work and thanks. Closing.