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

PhET-iO code review #299

Closed
jbphet opened this issue Jun 23, 2020 · 9 comments
Closed

PhET-iO code review #299

jbphet opened this issue Jun 23, 2020 · 9 comments

Comments

@jbphet
Copy link
Contributor

jbphet commented Jun 23, 2020

PhET-iO instrumentation has been added to this sim for #245, and part of the process is to have a code review. However, there is an impending dev release to the client (see #296), and in a Slack conversation @kathy-phet said she did not want to delay the client release in order to complete the code review.

@samreid - I thought I'd start by assigning this to you, you can re-assign as you and the phet-io team see fit. It all likelihood this will go on in parallel with the client's evaluation of the dev version.

@samreid samreid removed their assignment Jun 23, 2020
@samreid
Copy link
Member

samreid commented Jun 23, 2020

Thanks, I labeled it for PhET-iO Wednesday meeting for discussion.

@zepumph
Copy link
Member

zepumph commented Jun 25, 2020

@samreid volunteered. Thanks! This doesn't blockdev, and so should be done at a medium to medium/high priority as it will block RC publication.

@samreid
Copy link
Member

samreid commented Jun 26, 2020

I labeled it high to put it on my radar, but likely I shouldn't make any commits until there is a dev version.

@zepumph
Copy link
Member

zepumph commented Jun 26, 2020

A dev version has been made for SOM! See #296.

@samreid
Copy link
Member

samreid commented Jun 29, 2020

@jbphet can you please identify the scope for this review? Is all of the code under states-of-matter/js and states-of-matter-basics/js and atomic-interactions/js? Since this has passed design team and dev testing, will I need to review any runtime parts, or is this all about implementation?

@jbphet
Copy link
Contributor Author

jbphet commented Jun 30, 2020

@samreid asked:

[C]an you please identify the scope for this review? Is all of the code under states-of-matter/js and states-of-matter-basics/js and atomic-interactions/js? Since this has passed design team and dev testing, will I need to review any runtime parts, or is this all about implementation?

There isn't much code in states-of-matter-basics and atomic-interactions, since they both derive entirely from states-of-matter, so I'd like to request that yes, you code review those as well, but I don't think that there will be much to it. Let me know if you'd like separate issues for this, but it seems unnecessary to me.

I'm not entirely sure what you mean by "[W]ill I need to review any runtime parts, or is this all about implementation?" I'm thinking that you're looking through the code that was added to support phet-io and making sure that I've followed the latest procedures and recommendations for how this is done, and also checking that I haven't done anything that will cause problematic behavior or make maintenance more difficult than it has to be. I don't think you need to do any testing of the sim. If that doesn't answer your question, let's set up a Zoom call to clarify.

@samreid
Copy link
Member

samreid commented Jul 1, 2020

I finished the code review, and everything looks in really good shape. From the code review document:

At the very least, be sure to know what amount of instrumentation this sim supports. Describing this further goes beyond the scope of this document.

Is the simulation feature-complete for a production version of PhET-iO? After this dev version could we move forward to RC, or is there other work that would be required first?

From the PhET-iO Instrumentation Guide

  • Bring the sim up to PhET code standards, including conversion to ES6 (classes, arrow functions, etc.)

Another part said

A nice way to check state is to look at the "changed state" feature in the state wrapper.

I noticed 2200 "null" values in the state wrapper after starting up one of the screens. Is this as expected?

  • 5 // REVIEW comments annotated in the code
  • 5 new issues referenced above

samreid added a commit that referenced this issue Jul 1, 2020
@samreid samreid removed their assignment Jul 1, 2020
@jbphet
Copy link
Contributor Author

jbphet commented Jul 30, 2020

The REVIEW comments have all been addressed, as have the issues that resulted from the review. Some of those issues were closed and some were left open to make sure that they were tested during the RC process. @samreid - back to you to review the commits and issue responses and either close or send back to me if you feel that there is more to be done on this.

@samreid
Copy link
Member

samreid commented Aug 4, 2020

I confirmed the REVIEW comments have been addressed nicely, thanks! I think the last issue is #307, we will finish there. Nice work @jbphet, closing.

@samreid samreid closed this as completed Aug 4, 2020
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