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

Factor out/minimize overrides files. #303

Closed
samreid opened this issue Jun 29, 2020 · 16 comments
Closed

Factor out/minimize overrides files. #303

samreid opened this issue Jun 29, 2020 · 16 comments

Comments

@samreid
Copy link
Member

samreid commented Jun 29, 2020

Discovered during #299. The overrides files must be maintained along with the simulation, and things will be simpler if we can minimize them as much as possible.

This can be done safely by comparing grunt generate-phet-io-api before and after a change. I'll take a look.

@samreid samreid self-assigned this Jun 29, 2020
samreid added a commit to phetsims/atomic-interactions that referenced this issue Jun 29, 2020
samreid added a commit to phetsims/atomic-interactions that referenced this issue Jun 29, 2020
samreid added a commit to phetsims/atomic-interactions that referenced this issue Jun 29, 2020
samreid added a commit to phetsims/states-of-matter-basics that referenced this issue Jun 29, 2020
samreid added a commit to phetsims/atomic-interactions that referenced this issue Jun 29, 2020
samreid added a commit to phetsims/states-of-matter-basics that referenced this issue Jun 29, 2020
samreid added a commit to phetsims/atomic-interactions that referenced this issue Jun 29, 2020
samreid added a commit to phetsims/atomic-interactions that referenced this issue Jun 29, 2020
samreid added a commit to phetsims/states-of-matter-basics that referenced this issue Jun 29, 2020
@samreid
Copy link
Member Author

samreid commented Jun 29, 2020

I've committed several changes factoring out overrides from several places into the point of origination, I was able to do around one per minute (I focused on the ones that did not require simulation-specific knowledge), and it seemed a good use of time. For instance, atomic interactions is down from 27 to 19 overrides. I recommend taking a bit more time to look through and see if anything else is easy to factor out.

@jbphet
Copy link
Contributor

jbphet commented Jun 30, 2020

@samreid - I have a few questions, and a comment, on this:

  • It looks like you moved some overrides and not others. In your comment you said, "I recommend taking a bit more time to look through and see if anything else is easy to factor out." Is the goal to move them all? If so, what is the purpose of the overrides file? If not, what are the guiding principles for what should be in the overrides file and what should be in the code?
  • I ran grunt generate-phet-io-api as you suggested about, and it generated a file that includes a number of differences that look like they come from common code. Also, the format of the file name seems different from what's in the directory now, and the file seems to include a date a time in the name (2020-4-30_11.20.41.json versus 6-30-2020_1.39.04_PM.json). Am I supposed to check these in on changes, or is the idea just to compare them and look for the changes that I intended to make (and to ignore things from common code)?
  • Finally, just a heads up: The changes for this caused CT errors in atomic-interactions and states-of-matter-basics, so if you do anything else on this, please make sure to check those sims as well. See CT Any schema entries in the overrides file must be different from its baseline counterpart atomic-interactions#87 and CT Any schema entries in the overrides file must be different from its baseline counterpart states-of-matter-basics#28.

@jbphet jbphet assigned samreid and unassigned jbphet Jun 30, 2020
samreid added a commit to phetsims/atomic-interactions that referenced this issue Jul 3, 2020
samreid added a commit to phetsims/atomic-interactions that referenced this issue Jul 3, 2020
samreid added a commit to phetsims/atomic-interactions that referenced this issue Jul 3, 2020
samreid added a commit to phetsims/atomic-interactions that referenced this issue Jul 3, 2020
samreid added a commit to phetsims/atomic-interactions that referenced this issue Jul 3, 2020
samreid added a commit to phetsims/atomic-interactions that referenced this issue Jul 3, 2020
samreid added a commit to phetsims/atomic-interactions that referenced this issue Jul 3, 2020
samreid added a commit to phetsims/atomic-interactions that referenced this issue Jul 3, 2020
samreid added a commit that referenced this issue Jul 3, 2020
samreid added a commit that referenced this issue Jul 3, 2020
samreid added a commit that referenced this issue Jul 3, 2020
samreid added a commit that referenced this issue Jul 3, 2020
samreid added a commit that referenced this issue Jul 3, 2020
samreid added a commit that referenced this issue Jul 3, 2020
This reverts commit b2cfefa.

# Conflicts:
#	js/atomic-interactions/model/DualAtomModel.js
samreid added a commit that referenced this issue Jul 3, 2020
samreid added a commit that referenced this issue Jul 3, 2020
samreid added a commit that referenced this issue Jul 3, 2020
samreid added a commit that referenced this issue Jul 3, 2020
samreid added a commit that referenced this issue Jul 3, 2020
@samreid
Copy link
Member Author

samreid commented Jul 3, 2020

I reverted the changes and used grunt generate-phet-io-api to confirm that the overall API was not disturbed. @jbphet can you please review?

@jbphet
Copy link
Contributor

jbphet commented Jul 8, 2020

I reviewed this by skimming through the overrides file history for SOM, SOMB, and AI, and they look pretty reasonable to me. The only thing in them is the phetioFeatured settings and they look pretty close to what I remember. The featured items will likely get reviewed before final publication too, so I think this is safe to close.

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

4 participants