pixelzoom commented 2 years ago

239 (add support for dynamic locale) will certainly result in many API changes. According to the PhET-iO spreadsheet, this sim needs to provide migration support from 1.5 to 1.6. So migration rules will be needed.

Note that this also includes ph-scale-basics.

Previous versions, for generating *-custom-wrapper.html files:

samreid commented 2 years ago

I'll reassign this when we get closer to that stage. Self-unassigning for now.

samreid commented 1 year ago

@pixelzoom said on slack:

If pH Scale is indeed up next, the PhET-iO team might want to take a look at https://github.com/phetsims/ph-scale/issues/240 (Migration rules for 1.6 release). My understanding is that it’s a blocking issue, on-hold until PhET-iO design review is completed in https://github.com/phetsims/ph-scale/issues/241. (edited)

arouinfar commented 1 year ago

Design review has been completed in #241. As far as I'm aware, there's nothing blocking this issue.

samreid commented 1 year ago

I added migration rules for ph-scale and ph-scale-basics. @zepumph and I can review/discuss/improve the rules, but they seem equal to the quality of the rules approved for gravity and orbits, so this issue is no longer blocking an RC.

pixelzoom commented 1 year ago

My understanding is that part of this process is to familiarize developers with migration rules. @samreid Can you give me an overview of rules? -- specifically: how you identified them, how you implemented them, and how to review them.

samreid commented 1 year ago

Discussion with @pixelzoom and @zepumph:

How to deal with breaking changes in minor versions? Not possible at the moment. How to factor out rules across sims? Don't do it at the moment. When you change an API file, it goes with a migration rule (or deleting a migration rule)

20 rules across sims in order

{ myRule: rule, timestamp: dec 6, 2022 }

Assumption that migration versions are MAJOR.MINOR: publish 1.5

publish 1.6.0 publish 1.6.1

publish 1.0 publish 1.1 1.2 is on master

client needs 1.0.1 with a breaking API change

enhancedSound => extraSound extraSound => enhancedSound enhancedSound => extraSound

identical to enhancedSound => extraSound

Update: we would like to use MAJOR.MINOR.MAINTENANCE for the keys, and allow migration between maintenance versions.

Please note that https://github.com/phetsims/phet-io/issues/1861 outlines the testing needs from this conversation.

samreid commented 1 year ago

In https://github.com/phetsims/phet-io/issues/1899#issuecomment-1341198162 we said:

Yesterday in a meeting with @pixelzoom @zepumph and myself, we concluded that migration rules should be versioned by MAJOR.MINOR.MAINTENANCE and that this would be a blocking feature for getting ph-scale and ph-scale-basics into RC. We did not think that would be necessary for maintenance releasing into gravity-and-orbits, but we may change our mind about that after we see how it goes.

samreid commented 1 year ago

@zepumph and I closed https://github.com/phetsims/phet-io/issues/1899#issuecomment-1341198162, so this issue is either ready to be closed, or ready for review by @pixelzoom. @pixelzoom what do you recommend?

pixelzoom commented 1 year ago

I'm going to assume that this was rigorously tested by the developers who wrote the migration rules, and any remaining issues will be identified by QA. So closing this issue.

pixelzoom commented 1 year ago

Reopening. There's more work to be done here.

[x] Address general problems for https://github.com/phetsims/phet-io-sim-specific/issues/17
[x] Review https://github.com/phetsims/ph-scale/issues/260

Since a major feature of the 1.6 release is migration, I expect the -migration-rules.ts files to be production quality. That's not currently the case. After I cleaned up -migration-rules.ts for https://github.com/phetsims/ph-scale/issues/260, @samreid added some things that are not production quality.

[x] Remove dead code, like this:

   // new TandemFragmentRenamed( `${simName}.general.view.navigationBar.titleText.textProperty`, `${simName}.general.model.strings.${simName}.${simName}.titleStringProperty` ),
    new TandemFragmentRenamed( 'phScale.general.view.navigationBar.macroScreenButton.text.textProperty', 'phScale.general.model.strings.phScale.screen.macroStringProperty' ),
    // new TandemFragmentRenamed( `${simName}.general.view.navigationBar.microScreenButton.text.textProperty`, `${simName}.general.model.strings.${simName}.screen.microStringProperty` ),
    // new TandemFragmentRenamed( `${simName}.general.view.navigationBar.mySolutionScreenButton.text.textProperty`, `${simName}.general.model.strings.${simName}.screen.mySolutionStringProperty` ),
    // new TandemFragmentRenamed( `${simName}.modelScreen.view.playAreaNode.starPlanetSceneView.timeCounter.clearButton.textNode.textProperty`, `${simName}.general.model.strings.${simName}.clearStringProperty` ),

[x] Address this confusing comment. Either add a TandemFragmentDelete rule, or make the comment clear.

    // 'phScale.macroScreen.view.pHMeterNode.pHIndicatorNode.numberDisplay.valueText.textProperty' no longer customizable

[x] Are we ordering this file by common rules at the top, followed by sim-specific rules? If not, delete this comment:

    //////////////////////////////////////////////////////
    // ph-scale specific rules

samreid commented 1 year ago

I'll take a look...

pixelzoom commented 1 year ago

12/22/2022 phet-io meeting

@samreid addressed #260. Back to me to review the migration-rules.ts files.

pixelzoom commented 1 year ago

I reviewed ph-scale-migration-rules.ts and ph-scale-basics-migration-rules.ts. Looks OK to proceed, and tested OK in 1.5 to master (1.6) migration. Closing.

phetsims / ph-scale

Migration rules for 1.6 release #240

239 (add support for dynamic locale) will certainly result in many API changes. According to the PhET-iO spreadsheet, this sim needs to provide migration support from 1.5 to 1.6. So migration rules will be needed.