Identify unused strings in sims

[samreid]

In phetsims/bending-light#340 I discovered that there were unused strings in Bending Light. We should add an automated tool to report on this (I had thought there was one?) or add steps in the code review to prevent this problem in the future because once a simulation is published it becomes difficult to change the strings file (due to concurrent modification by translators).

[samreid]

I added a checklist item, which can be seen here:
https://github.com/phetsims/phet-info/blob/master/checklists/code_review_checklist.md

Should we also try to create automated tools to help with this (or are there already some in place that I am unaware of)?

[pixelzoom]

Does Rosetta show these unused strings to the translator, or does it get the set of strings that are actually used from the html file? If the latter, then I care less about this issue.

[pixelzoom]

@samreid says that rosetta.routes.js, line 407, looks like it's reading the English strings file, and therefore showing unused strings.

[pixelzoom]

So we need something like chipper.reportUnusedMedia, but for strings. As strings are loaded, add them to a list. When done, compare the list to the English string file, identify English keys that are not in the list - they are the unused strings.

[pixelzoom]

@jessegreenberg volunteered to looks at this.

[jessegreenberg]

Example output from balloons-and-static-electricity:

[4mRunning "eslint:allFiles" (eslint) task[24m


[4mRunning "clean" task[24m

[4mRunning "requirejs:build" (requirejs) task[24m
[32m>> [39mRequireJS optimizer finished

[4mC:/Users/Jesse/Development/phetsims/balloons-and-static-electricity/build/balloons-and-static-electricity.min.js[24m
Uncompressed size: [32m2212199[39m bytes.
Compressed size: [32m343958[39m bytes gzipped. ([32m963106[39m bytes minified)

[4mRunning "after-requirejs-build" task[24m
[31m>> [39mUnused string: BALLOONS_AND_STATIC_ELECTRICITY/rub.the.balloon, Rub the balloon
[31m>> [39mUnused string: BALLOONS_AND_STATIC_ELECTRICITY/BalloonApplet.Wall, Wall
[31m>> [39mUnused string: BALLOONS_AND_STATIC_ELECTRICITY/on.the.sweater, on the sweater
[31m>> [39mUnused string: BALLOONS_AND_STATIC_ELECTRICITY/BalloonApplet.IgnoreInitialBalloonCharge, Ignore Initial Balloon Charge
[31m>> [39mUnused string: BALLOONS_AND_STATIC_ELECTRICITY/bring.the.balloon, Bring the balloon
[31m>> [39mUnused string: BALLOONS_AND_STATIC_ELECTRICITY/near.the.wall, near the wall
[31m>> [39mUnused string: BALLOONS_AND_STATIC_ELECTRICITY/BalloonApplet.TwoBalloons, Two Balloons
[31m>> [39mUnused string: BALLOONS_AND_STATIC_ELECTRICITY/BalloonApplet.ChargeDisplay, Charge Display

[4mRunning "create-together-files" task[24m

[32mDone, without errors.[39m

[jessegreenberg]

Ran through grunt-all.sh, spot checking results. Reported unused strings were correct.

@pixelzoom, would you mind reviewing reportUnusedStrings.js if you have the time?

[pixelzoom]

Looks great. I made one change, which was to clarify the warning message. For example, for this unused entry in function-builder-strings_en.json:

  "foo": {
    "value": "Foo"
  }

The old message was:

>> Unused string: FUNCTION_BUILDER/foo, Foo

The revised message is:

>> Unused string: key=FUNCTION_BUILDER/foo, value=Foo

If that looks OK to you, feel free to close this issue.

[jessegreenberg]

Thanks for reviewing! Yes, that does clarify the output. Change is good, closing this issue.

[jbphet]

For the record, I think the assertion that rosetta uses the English string file as the basis of what to present to users of the translation utility (as stated in #460 (comment) above) is incorrect. I'm basing this on two things:

• There are nine translations for Bending Light that have been submitted in the last few days, and the published version of the sim is known to have about ten unused strings, and none of the translated files contain any of the unused strings.
• I know that a simulation's published HTML file is searched for strings from other repos, and only strings that are actually used from those repos are presented for translation, so it would be a little odd to not use that same approach for the sim's unique strings.

What could be going on at the aforementioned line 407 in rosetta/routes.js is that the English strings are being pulled from GitHub so that we know what to show the user for the English values, since these would be tricky to extract from the HTML file. In other words, keys are pulled from the HTML file, values are pulled from GitHub.

I could dig into the further if we need a definitive answer on this.

[pixelzoom]

Rosetta gets its strings from the published html file, so unused strings won't go away until a new sim version is published. A definitive answer to whether Rosetta presents unused strings to the translator is important only if PhET cares whether unused strings are being translated. Reopening and assigning to @ariel-phet to decide whether @jbphet should dig further.

[samreid]

Rosetta gets its strings from the published html file, so unused strings won't go away until a new sim version is published.

As far as I understand it, the strings are loaded with the requirejs plugin and hence only the used strings appear in the built HTML file.

[jbphet]

Since the currently published version of Bending Light (v1.0.0) is known to contain unused strings (see phetsims/bending-light#340), I figured that I'd test whether these strings appear on the translation page for this sim. The list of unused strings can be seen in this commit (where they were removed from the master branch): phetsims/bending-light@9922159.

Below is a screenshot of the translation page that was presented by Rosetta. None of the missing strings are presented. It's still a good idea to get rid of the unused strings as part of our development process, but in my opinion doing maintenance releases on existing sims with unused strings is not a good use of our time, since it appears that such strings aren't seen by translators.

[ariel-phet]

Marking for developer meeting just so we are all on the same page. If such strings are not seen by translators I am fine with making sure they are removed, but not worrying about doing maintenance releases to address, as the issue will get addressed with future redeploys (phet-io, a11y, etc)

[pixelzoom]

My understanding is:
(1) Unused strings are not presented to the translator by Rosetta, therefore
(2) there is no urgency to republish sims that contain unused strings, but
(3) as a matter of good code hygiene, unused strings should be removed when identified by the build process.

[jessegreenberg]

The above list makes summarizes my thoughts as well. No need to redeploy, but unused strings should be removed as they pop up after the build.