phetsims / perennial

Maintenance tools that won't change with different versions of chipper checked out
http://scenerystack.org/
MIT License
4 stars 5 forks source link

Suggestion for improvement to usage of getLocales #93

Open jbphet opened 6 years ago

jbphet commented 6 years ago

While working on #28, I noticed something that I thought was confusing in the usage of the getLocales function. The first portion of the function looks like this:

  async function getLocales( locales, simName ) {
  let callbackLocales;

  if ( locales && locales !== '*' ) {
    // from rosetta
    callbackLocales = locales;
  }
  else {
    .
    .
    .

There is only one call site and it looks like this:

    if ( api === '1.0' ) {
      locales = await getLocales( locales, simName );
    }

It seems quite odd to me that the function is basically short circuited unless the client passes in a value of '*' for locales. Based on its name and header description, I would expect it to always just get a list of the locales for a deployed simulation. Also, the variable name callbackLocales seems odd since I don't see anything related to callbacks in the function. I'd suggest removing the locales parameter and making the call site look something like this:

  if ( locales === '*' ){
     locales = getLocales( simName );
  }
jbphet commented 6 years ago

Assigning to @mattpen, since he has all of the commits on this file, though it looks like some history may have been lost prior to his work on this. He is welcome to reassign if appropriate.

jbphet commented 6 years ago

Pinging @zepumph since this is slightly related to issue #28, which he and I have worked on.