Code review for Locale updates (MR and main) #1441

jonathanolson · 2024-05-16T20:53:59Z

@samreid perhaps sometime it would be good to discuss how to review work done (with the goal of finding potential bugs without wasting your time).

Here is a fairly representative sampling of "major places where at least one different commit was made, likely contains something with large differences". It isn't comprehensive (a2dc4c1, phetsims/number-suite-common@0c92ece, 64846d9, 848e596 are examples - note the last one, as I fixed a BAD earlier "fix things for lint" commit).

UPDATE FROM MK: linking phetsims/joist#963 and phetsims/qa#1089

2024-05-02 balancing-act-1.3

2023-12-04 energy-skate-park-1.3

2023-04-13 number-compare-1.0

2022-12-24 ph-scale-1.6

2022-05-26 function-builder-basics-1.2

2021-07-22 john-travoltage-1.6

2020-01-28 gravity-force-lab-2.2

2019-10-23 vector-addition-equations-1.0

2019-06-05 fraction-matcher-1.2

2019-02-01 masses-and-springs-basics-1.0

2018-07-13 equality-explorer-two-variables-1.0

2018-01-24 capacitor-lab-basics-1.6

2017-11-14 gene-expression-essentials-1.0

2017-08-28 pendulum-lab-1.0

2017-01-25 make-a-ten-1.0

2016-11-22 concentration-1.5-phetio

2016-09-14 neuron-1.1

2016-04-14 energy-skate-park-basics-1.1

2015-10-01 hookes-law-1.0

Perhaps we could find a way to get my local commits up on the server somehow, since the below list is the finite list of cherry-picked things (SHAs are detached HEADs on my local working copies):

Maintenance Patches in MR: 
1.  [phet-io-sim-specific] https://github.com/phetsims/joist/issues/963
      2145526619e56cf5886f147f6e82a3b65fe3ed9d
      a8f51a7f136ff09e267f71ad7e4cda351c9ac944
      a70e07dcb3d27d99c5afaf787a182607aea23d3a
2.  [data] (chipper) https://github.com/phetsims/joist/issues/963
      2161f42b3ffb4be5e726d826cbf05778bf5b530c
      7e93bd250b4ec680ec0c5d72aed55990e347c363
      9fcaaa4121713c31f75db6c8a3e3ad7c853eea5f
      54dacd75c0d67a6f4a709e578be3afffd477e37a
      6f8c838a60cc4c5227307cd7cd41311678cdca72
      5d0878319e025878bca99e21984de4a3c2cc0bca
      dcfa25bf6ce6df32351193ea2d1f40d6ae81d109
      a029a3f8c86e8c697b499522b1eaef32be933d30
      e2898a9af7e626af3059f53081b463d7886a7333
      897bf03de99d23cfa6f1a9b7f5d5ec10c5845588
      d430746ce819e49d70dc7b8198a51882c96c59b8
      0a994016b43ffd886f525fa6f40dece2714435b4
3.  [lodash] (chipper) https://github.com/phetsims/joist/issues/963
      93c2876e08a12b4c867a787a42c437f8a8efb59a
4.  [lint-1] (chipper) https://github.com/phetsims/joist/issues/963
      41da5de2bea5bdaa35b7fdc931079fe7a6f91a85
5.  [spread-fix] (chipper) https://github.com/phetsims/joist/issues/963
      d59edbb00f7678ab289387acc2f85f4b4f05699f
6.  [remap] (chipper) https://github.com/phetsims/joist/issues/963
      036adceba5fe0c020f39ba60ea64e89d57f35216
      5cf6b54ea0be8712024326849ca37149cbc3c800
      f163619ee4b2c0787495c3b838b8a230e5be5aac
      c93aeccd891ead2a622ce888f29760d1b46285f0
      44550539b0492866df5741fa92498d573c3d8310
      fb4c3de8ae08a6c3c5c6e0bf0f3fbb22d8d803a5
      ba572594c09dc9014218b0d376bc92434dedeeb5
      b245f03f82af8366d52dac4cf25f75af1b214560
7.  [lint-2] (chipper) https://github.com/phetsims/joist/issues/963
      e04132a517fb28c8eb868c38ce18f15108dbde6b
      88598de9aba3d4f24e163c4229175e509ab375f9
8.  [lint-3] (chipper) https://github.com/phetsims/joist/issues/963
      29898bc4996318488042b214a1972242887dace4
9.  [permissive] (chipper) https://github.com/phetsims/joist/issues/963
      83461d239bd723877b955297edfca208318e0ee9
10. [warning] (chipper) https://github.com/phetsims/joist/issues/963
      96f24ef788abe73d521f85ae5c8ca533f319637f
11. [fallback-io] (joist) https://github.com/phetsims/joist/issues/963
      185625adc3db91573da6b752428ed3b2289580e2
      60aff7d95d384b1e58c0b61f72ce5e1cac74b8fa
12. [string-utils] (phetcommon) https://github.com/phetsims/joist/issues/963
      3d8102ba5a66bbe637793cb79ca1a0b52d7f497f
13. [i18n-replace] (joist) https://github.com/phetsims/joist/issues/963
      c0b66e2b487be54dcad564600d6b526ad26a5d87
      4fd461257ebd9831e44ee63c5dd74ccdf885dae2
14. [lang-select] (joist) https://github.com/phetsims/joist/issues/963
      4feee20b59927bfd5eda50556b4035cc2f7c42e6
      f831b2e7a294aa36274f04cde72b76546141b59c
      3d085e7695fea1e48ada19d08a25e4f3be259318
      608358ae841e52f3604986434b4679f0831977d1
      a5437469ce27fc6fc4e8221ea8a11bbe484e31d5
15. [locale-sort] (joist) https://github.com/phetsims/joist/issues/963
      621129232e80696b4cbf6c1db011f831626a7d2f
      f5ca6c52c97ded4272039fc86bcf67355435ca04
      ee9e1d13421c8776cbd42ae3cb1b9915ac292e15
16. [wrap-dirs] (phetcommon) https://github.com/phetsims/joist/issues/963
      6ec61b593351cdb8ae5c1ee25f1857919ca73419
      692ac60af543bac86bae4cb44e0ea00c6b686866
      3683668b9bda41cb6410625e13a938b0d68f5169
      6e1126f2e8a0d560f68faed31f57b385398c08e6
      14e0f0a64ffcc5b54587d81ce9a5a145aa3f250b
17. [wrap-dir] (phetcommon) https://github.com/phetsims/joist/issues/963
      cfc9e080e1cde614603d6763f0966a85cbe3577d
      b334f832dd9c4ed414b2d12b2a380f4d411ce0dc
      dc064da26feb7df0155f23de81a8d3d24feef53c
18. [string-module-info] (chipper) https://github.com/phetsims/joist/issues/963
19. [locale-order-property] (joist) https://github.com/phetsims/joist/issues/963
      05c397d2b376f25e84cd717183d550f946b51eda
      8de80150c904a8a978b8f66d8a9904a395f8d523
      a56157587620d2f80973804fe1c4422f309c5c6e
20. [lint-4] (joist) https://github.com/phetsims/joist/issues/963
      a8888a54be0ea24ca824aa43915ab4614f09d46d
21. [lstring] (chipper) https://github.com/phetsims/joist/issues/963
      64ee1c1e48cbbca5d61090ae7d181db3b3e45fce
      16e03c6df36cebc7e11ba600fef1a8b92a10588f
      3552332747e8edf8f703fe12e9806766c9dcd915
      b377e995dfa1f85920ce25cd486a82b7d19de6e4
      76c2b491e98cceb962fc04341080ff636384b923
      b0f42011c79399ddd936826f39e56bf28cf07bd2
22. [unbuilt] (chipper) https://github.com/phetsims/joist/issues/963
      2269be02e2fb0bdc65c90e4ae5a6624fdf0a3eb4
      50fe1763f0f1e1398e7824b1bd17b2e406085169
      bdb7de8d2c3d2300abf29347898f3b520559bc3f
      7208e14f3830a4547dc4421a8b9d24926ef4ea3b
      e09428ab9987bf372f2fa860e374b6a488b5fb4e
      9aa84d53c036f616986a052365ce16887593301e
      3c6dd1a0dba7c4fedc8e80d1aa5c64cb5aa109b3
      6c6e27a02b7966d155ff298a73e95ea386d13d4a
      b9338dce0ea81c4d05f3a4db30f9c9c519b1a938
      3e3f04f95b7851b1d7890f5646a988a3d9773777
      3ebbe1d0123e5c2bb997318d7465a35a7701d4bd
      36013996dd02680fe23c79a1d99fc934eb6a5588
23. [illegal-token] (chipper) https://github.com/phetsims/joist/issues/963
      85dc95546339d0556d0c3816d511ccbfab22f72b
24. [for-of-unbuilt] (chipper) https://github.com/phetsims/joist/issues/963
      49617dd80fe85ea6565ba46ea38055b8376b2813
25. [unused-vars-plugin] (chipper) https://github.com/phetsims/joist/issues/963
      1ee5755153a4f4ab0a00ab7117e04609de67dce6
26. [for-of-unused-plugin] (chipper) https://github.com/phetsims/joist/issues/963
      019b50b0c6cf5095927438d64abb2c2b41386714
27. [lstring-undef] (chipper) https://github.com/phetsims/joist/issues/963
      2837914cc67dce6756540c0121dc89d518e9983c
      b3cc7dd28da3231462f4cd5d2b67672092bedf08
28. [string-concat] (chipper) https://github.com/phetsims/joist/issues/963
      62f9ae81922c994a0bf35eeaf63ef39fd27b298c
29. [illegal-token-2] (chipper) https://github.com/phetsims/joist/issues/963
      451c9d1ab0485d6faad70590c25a00b7c96690de
30. [post-token] (chipper) https://github.com/phetsims/joist/issues/963
      7367d73d3dbe44b101c47b977f76f122f6273dfd
      3964fb541184a9cb498d9881cecb532b5ad762bb
31. [bad-string] (chipper) https://github.com/phetsims/joist/issues/963
      3d45b97e33ed2415138ef7ab1adde5e95b020985
32. [nsc] (number-suite-common) https://github.com/phetsims/joist/issues/963
      47d3c0cd4936ec804bdb005426e7816c0d9c0069

The text was updated successfully, but these errors were encountered:

samreid · 2024-05-17T13:59:07Z

We have a calendar entry to look at this today with me @jonathanolson and @zepumph, self-unassigning until then.

zepumph · 2024-05-20T20:34:36Z

We had another conversation. This time it centered around having better processes for realizing code reviews in MRs when needed. Here are some steps we'd like to take moving forward:

Improvements to Maintenance releases (big goal to make it easier to get an MR reviewed!)

most automated testing possible (across release branches)
Review of main eagerly as the changes are on main (even if it doesn't block the beginning of cherry picking)
Ability to push detached commits via git tags (automatically via the MR process). @jonathanolson mentioned perhaps while working on his next MR so there is adequate work to test and prototype with.
Use of diffing the commits (diff of diffs) to see how the patch applies to older and older changes
Apply patches on sims from newest to oldest! (update tooling to default sort by release date instead of alphabetically). With an alphabetical option if desired (but not default)

We also co-reviewed the changes on main on Friday. From here. @jonathanolson has a large list of automated tests the we came up with from our last two meetings. He will work on those to increase our confidence.

I will look through and spot check a couple of older change sets and report back

…L, see phetsims/joist#963

…hetsims/joist#963

…st#963

Signed-off-by: Michael Kauzmann <[email protected]>

zepumph · 2024-05-20T22:35:52Z

I looked through hookes law 1.0, masses and springs 1.0, and main.

I commented in some commits in individual branches, I made review comments inline in main, and I have a few items here:

Review:

On main:

Instead of adding computation logic in getInitializationScript, can we just provide the right list to begin with? We already have computed fallback locales into the stringMap generally
- Move logic out to getPrunedLocaleData.js
I'd like to better understand where the logic is that enforces that babel stays at the tip of main. Can we add an automated test on all release branches that after running the same (ish) codepath that build server would run to build a release, that babel is still on main? Perhaps this is just redundant to the test where we actually add a locale on main and make sure all releases pick it up.
validInitialLocale in localeProperty, why do we need it when we also have initialize global logic.
Is there ever a case where we depend on localData global in the sim being only the things translated (+ fallback)?

Is localeOrderProperty really ideal? Does it have to be duplicated with

chipper/js/LocalizedString.ts

Lines 105 to 109 in c16eff4

    
           const orderedLocales: Locale[] = [ 
        
             locale, 
        
             ...( localeData[ locale ].fallbackLocales || [] ), 
        
             FALLBACK_LOCALE 
        
           ];

?

Review comments
TODOs pointing to this issue

GENERAL thoughts:

Maybe commit messages for auto MR patches should have a schema to make reviewing commits easier. Name them for the MR patch name perhaps?
In general I feel like the change set has a plethora of spots that need to care about the fallback locales. I wish that could be something more like twice, or maybe just once. I understand not wanting to dive into that for changes that are going into release branches, but can we work out a bit more of a direct algorithm for when we absolutely NEED to outline fallbacks.
confirming that Hookes Law (and so likely other old sims) didn't have any changes for this problem in joist. I guess because the string plugin code was all in chipper. Do I have that right?

jonathanolson · 2024-05-20T23:20:09Z

I'd like to better understand where the logic is that enforces that babel stays at the tip of main.

build-server using updateCheckout: https://github.com/phetsims/perennial/blob/78025b7ae6064e9ab5260cea5e532f3bf24c3ec8/js/build-server/taskWorker.js#L159
babel override dependency: https://github.com/phetsims/perennial/blob/9b3eeeab170915cae99e62a27b743b12f21a2ee6/js/common/ReleaseBranch.js#L217 defaulting to main https://github.com/phetsims/perennial/blob/610c97d5a0204e9e822e83a02e6c4794e32c982e/js/build-server/getBuildServerConfig.js#L53
babel pull: https://github.com/phetsims/perennial/blob/9b3eeeab170915cae99e62a27b743b12f21a2ee6/js/common/ReleaseBranch.js#L234-L236

jonathanolson · 2024-05-20T23:21:56Z

Can we add an automated test on all release branches that after running the same (ish) codepath that build server would run to build a release, that babel is still on main?

Yes, might as well toss that in, I think it will be cheap.

Perhaps this is just redundant to the test where we actually add a locale on main and make sure all releases pick it up.

Not redundant, I can't commit that to main until the MR is complete.

jonathanolson · 2024-05-20T23:45:08Z

validInitialLocale in localeProperty, why do we need it when we also have initialize global logic.

If we have no es_PY translation (but have the fallback es translation), initialize-globals currently sets phet.chipper.locale to es_PY, and localeProperty remaps it to es (since es_PY is considered "invalid" because it is not a valid value of the localeProperty).

Signed-off-by: Michael Kauzmann <[email protected]>

zepumph · 2024-05-21T19:28:06Z

validInitialLocale in localeProperty, why do we need it when we also have initialize global logic.

If we have no es_PY translation (but have the fallback es translation), initialize-globals currently sets phet.chipper.locale to es_PY, and localeProperty remaps it to es (since es_PY is considered "invalid" because it is not a valid value of the localeProperty).

That is a helpful explanation. I confirmed the behavior with console logs locally, but can you explain why this is desirable? Is this complexity exclusively because we can't (or don't want to) hold up execution of initialize-globals before we have loaded unbuilt strings? I don't think that is appropriate. Happy to discuss more.

jonathanolson · 2024-05-22T22:44:44Z

That is a helpful explanation. I confirmed the behavior with console logs locally, but can you explain why this is desirable? Is this complexity exclusively because we can't (or don't want to) hold up execution of initialize-globals before we have loaded unbuilt strings? I don't think that is appropriate. Happy to discuss more.

Yes, it is basically waiting for the unbuilt strings to load. If we delay most of the initialize-globals logic until AFTER strings have loaded, it would work to simplify the locale handling and DIRECTLY compute the final locale.

Presumably just how sim dev HTMLs have a global loadModules() callback, we could also have a loadedStrings() callback() that kicks off loading of things that need this.

We'd need to analyze what other preloads rely on initialize-globals running. If NONE rely on it, we could rewrite initialize-globals to create the loadedStrings() hook (and IT would be responsible for calling loadModules(). If not, we'd have to choose between splitting our preloads into "needs initialize globals" and "doesn't need", OR just waiting to load all preloads.

Thoughts?

jonathanolson · 2024-05-22T23:35:45Z

Is there ever a case where we depend on localData global in the sim being only the things translated (+ fallback)?

I don't think so. We just try to use the data when available.

jonathanolson · 2024-05-22T23:36:19Z

Is localeOrderProperty really ideal? Does it have to be duplicated with

Not ideal, we might be able to get rid of localeOrderProperty soon, it doesn't have much use.

zepumph · 2024-06-07T22:20:28Z

Over in https://github.com/phetsims/phet-io/issues/1881#issuecomment-2155615116 @samreid and I found a couple more oddities in main to check in about:

getStringForBuiltSim has no usages.
phet.chipper.queryParameters.locales is not used anymore.

zepumph · 2024-06-26T21:27:50Z

Also noting multiple related checkboxes in More locale work: localeProperty should be graceful and more central joist#970 (comment) that should be done as part of this review.

Signed-off-by: Michael Kauzmann <[email protected]>

…hetsims/chipper#1441

…sims/chipper#1441

jonathanolson · 2024-07-15T17:31:29Z

Handled all of the above items! Closing.

jonathanolson added the dev:code-review label May 16, 2024

jonathanolson assigned samreid May 16, 2024

samreid removed their assignment May 17, 2024

zepumph assigned samreid and zepumph May 17, 2024

zepumph assigned jonathanolson and unassigned samreid May 20, 2024

zepumph referenced this issue in phetsims/phetcommon May 20, 2024

Adding StringUtils.wrapDirection/localeToLocalizedName, fixing wrapRT…

8953fd7

…L, see phetsims/joist#963

zepumph referenced this issue May 20, 2024

Unbuilt loading of phet.chipper.localeData and string handling, see p…

8851aaf

…hetsims/joist#963

zepumph referenced this issue May 20, 2024

Adding locale remapping/insensitivity to built form, see phetsims/joi…

76a86a7

…st#963

zepumph added a commit that referenced this issue May 20, 2024

review comments, #1441

9f18c9e

Signed-off-by: Michael Kauzmann <[email protected]>

zepumph removed their assignment May 20, 2024

zepumph added the status:ready-for-review label May 20, 2024

zepumph added a commit that referenced this issue May 21, 2024

assert messages, #1441

d03dbc1

Signed-off-by: Michael Kauzmann <[email protected]>

This was referenced Jun 10, 2024

More locale work: localeProperty should be graceful and more central phetsims/joist#970

Closed

Locale Query Parameter taking long values like "espan" and loading like it is "es" phetsims/joist#963

Closed

jbphet mentioned this issue Jun 12, 2024

Locale info not getting updated very often on live translation utility phetsims/rosetta#431

Closed

zepumph mentioned this issue Jun 17, 2024

Locale MR 2 - PhET-iO Hydrogen Standard Wrapper changes phetsims/qa#1098

Closed

74 tasks

zepumph changed the title ~~Code review (spot check?) for locale MR~~ Code review for Locale updates (MR and main) Jun 26, 2024

zepumph added a commit to phetsims/perennial that referenced this issue Jun 28, 2024

update TODO to review issue, phetsims/chipper#1441 phetsims/joist#963

c5bbc42

Signed-off-by: Michael Kauzmann <[email protected]>

zepumph added a commit that referenced this issue Jun 28, 2024

update TODO to review issue, #1441 phetsims/joist#963

07cf9ee

Signed-off-by: Michael Kauzmann <[email protected]>

zepumph mentioned this issue Jul 8, 2024

Spanish "es" translation of Build an Atom is not opening (the sim open in English) phetsims/build-an-atom#242

Closed

jonathanolson added a commit that referenced this issue Jul 8, 2024

Removing unused query parameters locales/strings, see #1441

0810d71

mattpen pushed a commit to phetsims/perennial that referenced this issue Jul 8, 2024

changed translations to always build all locales on the build-server p…

fb5e3a1

…hetsims/chipper#1441

jonathanolson added a commit to phetsims/perennial that referenced this issue Jul 8, 2024

spacing fix, see phetsims/chipper#1441

0656033

jonathanolson added a commit to phetsims/perennial that referenced this issue Jul 8, 2024

Adding redeployment to ReleaseBranch (from production SHAs), see phet…

426d009

…sims/chipper#1441

jonathanolson added a commit that referenced this issue Jul 15, 2024

Handling review comment, see #1441

96402c8

jonathanolson added a commit to phetsims/perennial that referenced this issue Jul 15, 2024

updating parseScreenNames to use localeData, see phetsims/chipper#1441

7e0668a

jonathanolson added a commit that referenced this issue Jul 15, 2024

Adding getPrunedLocaleData, see #1441

d9c7d31

jonathanolson added a commit that referenced this issue Jul 15, 2024

Removing disabling of updateLocaleInfo, see #1441

8e41c7f

jonathanolson added a commit that referenced this issue Jul 15, 2024

updating parseScreenNames to use localeData, see #1441

6f6935b

jonathanolson closed this as completed Jul 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code review for Locale updates (MR and main) #1441

Code review for Locale updates (MR and main) #1441

jonathanolson commented May 16, 2024 •

edited by zepumph

Loading

samreid commented May 17, 2024

zepumph commented May 20, 2024 •

edited

Loading

zepumph commented May 20, 2024 •

edited by jonathanolson

Loading

jonathanolson commented May 20, 2024

jonathanolson commented May 20, 2024

jonathanolson commented May 20, 2024

zepumph commented May 21, 2024 •

edited

Loading

jonathanolson commented May 22, 2024

jonathanolson commented May 22, 2024

jonathanolson commented May 22, 2024

zepumph commented Jun 7, 2024 •

edited by jonathanolson

Loading

zepumph commented Jun 26, 2024

jonathanolson commented Jul 15, 2024

Code review for Locale updates (MR and main) #1441

Code review for Locale updates (MR and main) #1441

Comments

jonathanolson commented May 16, 2024 • edited by zepumph Loading

samreid commented May 17, 2024

zepumph commented May 20, 2024 • edited Loading

zepumph commented May 20, 2024 • edited by jonathanolson Loading

jonathanolson commented May 20, 2024

jonathanolson commented May 20, 2024

jonathanolson commented May 20, 2024

zepumph commented May 21, 2024 • edited Loading

jonathanolson commented May 22, 2024

jonathanolson commented May 22, 2024

jonathanolson commented May 22, 2024

zepumph commented Jun 7, 2024 • edited by jonathanolson Loading

zepumph commented Jun 26, 2024

jonathanolson commented Jul 15, 2024

jonathanolson commented May 16, 2024 •

edited by zepumph

Loading

zepumph commented May 20, 2024 •

edited

Loading

zepumph commented May 20, 2024 •

edited by jonathanolson

Loading

zepumph commented May 21, 2024 •

edited

Loading

zepumph commented Jun 7, 2024 •

edited by jonathanolson

Loading