Project-wide common code review (a11y/phet-io)

[jonathanolson]

Summary (for making changes):

• Before you work on changes, look at outstanding work or problems related to the code.
• Before adding phet-io or a11y, evaluate and understand what is about to be added. Is there a good fit between the current implementation and what you need to do? (If not, decide what the scope of changes should be)
• As you're adding features, all of the typical code review items apply.
• Someone needs to review the code changes when it is done (or pairs during the process)
• Maintain top-level comments that indicate the implementation status of the file for phet-io/a11y.

[samreid]

Maintain top-level comments that indicate the implementation status of the file for phet-io/a11y.

Equally or more importantly, identifying which files require significant refactoring/rewriting/modularization before it is appropriate to instrument.

[zepumph]

Maintain top-level comments that indicate the implementation status of the file for phet-io/a11y.

Before diving in too much about this, I think it would be good to flush out what the type of comment would be at the top of files. I think it would be important to not be so specific that it becomes hard to maintain, but also it should specify enough information that someone doesn't still need to check in the code. If that balance can't be found maybe this would just be a wasted cost.

[jbphet]

More thoughts from the 1/31/2019 dev meeting:

• In general, when a developer comes across a common code component that isn't in shape to have phet-io or a11y added to it, they should suck it up and to do the work necessary to do it well rather than just graft something done in order to get it working
• Consider doing this sort of work in a branch to avoid destabilizing master, but don't allow branches to linger for long periods of time to avoid merge conflicts
• Common but not widely used components (such as BicyclePumpNode), it's generally fine to do it in master, with a very visible comment in the header about the state of affairs
• Another option is to make a new version of the component and essentially start over. This can be done on master since presumably no sims would be using it yet.

[jessegreenberg]

In addition to the above list, we discussed that the developer instrumenting should consult with the original author of the component to go over new features, approaches, and discuss current state of the component. What may seem like a reasonable solution to the one instrumenting may feel more like grafting to the original author.

This issue was originally discussed because developers were surprised to find large changes in components they had worked on. I think going forward points in the above comments will prevent this in the future. @ariel-phet I feel this has been discussed enough and can be closed.

[oliver-phet]

@chrisklus Will create google spreadsheet for each common code component (sun, scenery-phet, joist): repo, responsible dev, priority, condition, phet-io, accessibility, sonification
SR: maybe look around Binder as a reference
JG: EM or TS may already have a list of these components

[pixelzoom]

5/16/19 dev meeting update from @chrisklus: All of his time was taken up on other things this week, so maybe next week.

[chrisklus]

First pass here: https://docs.google.com/spreadsheets/d/1f3MN7J57deJIN6dGhZ3lbK5Zwbo2vyrgXPu0AmRjCR0/edit#gid=0

I came up with the fields and created a key, and then per @pixelzoom's suggestion, only filled out four components so the team can give feedback and discuss the chart setup.

[pixelzoom]

Self assigning so I remember to review.

[zepumph]

I definitely like the way we are starting to keep track of this state of common code elements. This is especially helpful as our project grows, and with that the footprint of a11y and phet-io do too.

That said I'm not really understanding the purpose of a standalone spreadsheet for this. I can't seem to think of a way in which this won't get totally out of sync with the code. Perhaps if there was a way to annotate these fields within the main doc at the top of common code elements.

It seems autogeneration is the only way to make this document usable long term.

I think I was gone during the meeting where this was decided on, so forgive me if we already went through these concerns.

A few other thoughts about the columns:

• Status seems a bit too one-dimensional, I would hope we could combine them
• What is the purpose of the priority field?
• What is the work flow as it pertains to these rows and github issues? Do we create an issue when something is high enough priority for a particular status? Do we need to keep priorities in sync?

[pixelzoom]

@zepumph said:

... I'm not really understanding the purpose of a standalone spreadsheet for this.

I agree. It sure would be nicer to have this information at the top of the .js files. Is putting it in a spreadsheet intended to make it easier for non-developers to access? If so, what happened to documenting UI components in .md files, e.g. ComboBox.md ?

Seems like we're spreading info out over many places - .js files, .md files, "visual demos" in sun/scenery-phet,... Maybe we should hold off on the spreadsheet until we've discussed further?

[samreid]

Yes, let's discuss further before more work.

[samreid]

Today @ariel-phet proposed writing a tool that automatically lists (a) all of the common code components used by a simulation and (b) listing all of the simulations a specific a common code component appears.

How can we unify this with the need to depict the level of a11y or phet-io instrumentation in each file?

Maybe if we know which simulations have been published with a11y or phet-io, that will help us know the level of instrumentation of each component (by cross-checking).

But we are discussing how to have a strategy that is flexible enough and won't immediately get stale.

[samreid]

When building a sim, we need to know:

(a) What existing common code components will it use? (we need a process for identifying these)

• for each common code component
  • make sure it has acceptable level of main/PhET-iO/a11y features (we need a process for identifying this)
  • if not, then working with the original author of the component, perform refactoring as necessary to make it amenable to main/PhET-iO/a11y for many sims.

(b) what new common code components will it introduce?

• make sure they are developed in a way that will be amenable to main/PhET-iO/a11y for many sims.

Starting out, we can have the "process" for each of those parts be driven by a designer+developer meeting. After several meetings we can identify which parts of those meetings can be automated or made more efficient.

[mattpen]

Top Goals:

• Designers would like to know which components in a sim are common code.
• Developers would like to know which sims have been instrumented for a11y and phet-io
• Developers would like to know which common-code components have been instrumented for a11y and phet-io
• When we are doing a11y/phet-io work how do we decide which components need updating? We need a process for making and documenting this decision.
• What is the process for making sure common code components are all of production quality?

Possible Solutions:

• Spreadsheet (consensus is this will not be maintainable)
• GitHub maintained list
• Machine generated report
• Simulation query parameter that will highlight components
  • Can we leverage the scenery inspector tool?

This will probably not be addressed on all components, but on a sim-by-sim basis. This probably needs to wait for Kathy for further discussion.

[mattpen]

@zepumph will investigate binder to see if it easily adaptable to these purposes.

[zepumph]

I have not yet gotten to this yet.

[zepumph]

In the above commits I added support for a list our sims, and each common code component used in it. You can see it here. http://phettest.colorado.edu/binder/doc/#sims

This exercise was useful in acquainting myself with the repo. It would be good to rank future priorities before doing more work.

Also note there is a bug where when you click on the side menu, it doubly adds the simulation view to the right pane.

My guess is that further tasks should have their own issues for them in binder as we continue to improve this doc project. All in all I think that binder could be a very good solution for quite a few of the above goals. We just need to decide how we want each to pan out.

[zepumph]

From dev meeting this week, it was brought up that a potential solution for phet-io and a11y documentation is to use the InstanceRegistry to mark runnable "code documentation" that bind can parse and display. We wanted to wait for @samreid before continuing that conversation. In the mean time I will work on phetsims/binder#25.

[samreid]

We wanted to wait for @samreid before continuing that conversation.

Can you please clarify next steps for me?

[samreid]

From dev meeting notes:

AP, CM: Schedule some time to bring common code up to the latest PhET-iO standards. E.g. use of type-specific Properties, phetioType, uninstrument dynamic elements. Create an issue in tasks repo?
MK: We should discuss documentation strategy, like how this may be covered in binder.
AP: Perhaps we can codify a level like “PhET-iO 1.0”. Then we can use that as a standard for determining which things are brought up to that level.
MK: Can we elaborate on the purpose of this undertaking? Things will still be churning for a while.
AP: This helps in understanding what work needs to be done in common code for a new simulation.
AP: Visiting common code on a sim-by-sim basis seems reasonable and realistic.
SR: A first order approximation would be to see which common code components have appeared in a published PhET-iO simulation.
MK: That makes sense as a heuristic that doesn’t require a new maintained artifact.

[zepumph]

Next steps for this issue is to develop, in <2 hours or so, a prototype that gathers a list of components that have been published in a phet-io version since GQIO 1.1. We will consider those "up to phet-io standard," and all others as needing attention.

[jonathanolson]

@ariel-phet can we just remove this from the developer meeting, or is there something additional to do?

[chrisklus]

Per discussion in 08/22/19 dev meeting, we are going to close this issue.

[zepumph]

@zepumph will create 2 issues from here:

1. go over the phet-io "how to instrument" document with the whole team for them to gain familiarity with phet-io code/process.
2. create a "how to instrument for a11y" document that outlines the steps and ordering and process for adding a11y.