Prevent naming mismatches when using the string plugin

[samreid]

During the Bending Light code review, @jessegreenberg pointed out several lines in Bending Light where there was a mismatch between the string key and the var declared in the require statement. For instance:

var miss = require( 'string!BENDING_LIGHT/miss' );

and

var c_units = require( 'string!BENDING_LIGHT/c_units' );

I wrote a eslint rule that requires the the following to be true: varName+'String' === keyName.

Here is the code for the rule:

// Copyright 2002-2015, University of Colorado Boulder
/**
 * @fileoverview Rule to check that a require statement assigns to the correct variable name for the string! plugin.
 * @author Sam Reid (PhET Interactive Simulations)
 * @copyright 2015 University of Colorado Boulder
 */

//------------------------------------------------------------------------------
// Rule Definition
//------------------------------------------------------------------------------

module.exports = function( context ) {
  'use strict';

  var endsWith = function( string, s ) {
    return string.length >= s.length && string.substring( string.length - s.length ) === s;
  };
  return {

    // Similar to the require-statement-match.js, please visit that file for AST example
    VariableDeclaration: function requireStatementMatch( node ) {

      if ( node.declarations &&
           node.declarations.length > 0 &&
           node.declarations[ 0 ].init &&
           node.declarations[ 0 ].init.arguments &&
           node.declarations[ 0 ].init.arguments.length > 0 ) {
        if ( node.declarations[ 0 ].init &&
             node.declarations[ 0 ].init.callee.name === 'require' ) {
          var lhs = node.declarations[ 0 ].id.name;
          var rhs = node.declarations[ 0 ].init.arguments[ 0 ].value;

          if ( rhs.indexOf( 'string!' ) === 0 ) {

            var lastSlash = rhs.lastIndexOf( '/' );
            var tail = rhs.substring( lastSlash + 1 );
            var lhsEndsWithString = endsWith( lhs, 'String' );
            var startsOK = lhs.indexOf( tail ) === 0;
            var midStringOK = lhs.length === (tail + 'String').length;
            if ( !lhsEndsWithString || !startsOK || !midStringOK ) {
              context.report( {
                node: node,
                loc: node.loc.start,
                message: 'Mismatched var in require(string!), ' +
                         'key=' + tail + ', ' +
                         'var=' + lhs
              } );
            }
          }
        }
      }
    }
  };
};

module.exports.schema = [
  // JSON Schema for rule options goes here
];

[samreid]

Thanks for your upvote @pixelzoom, I committed the rule to .eslintrc. @jessegreenberg @aaronsamuel137 @jonathanolson @jbphet @phetsims/aadish can you please help standardize the string vars using this lint output?

[samreid]

We've been making decisions like this to clean things up, and they are not getting done. Other examples include visibility annotations, fleshing out JSdoc, and namespacing.

It would be great if we could formalize some of these issues in eslint rules to make it easier to remember and check.

[samreid]

Since this is in master and we are going to share the burden of fixing the outstanding issues, I'll unassign.

[pixelzoom]

Let's add a 'developer-meeting' label so we can check in on the status of this.

[pixelzoom]

Here's a check list of repos that are currently failing this rule:

• [x] build-an-atom @jbphet
• [x] capacitor-lab-basics @jessegreenberg
• [x] curve-fitting
• [ ] energy-skate-park-basics @samreid
• [ ] fluid-pressure-and-flow @samreid
• [x] gene-expression-basics @jbphet
• [x] gravity-and-orbits
• [x] gravity-force-lab @jbphet (done by @pixelzoom)
• [x] isotopes-and-atomic-mass @jbphet
• [x] least-squares-regression @pixelzoom
• [x] making-tens
• [x] molecule-shapes
• [x] molecule-shapes-basics
• [x] neuron @jbphet (done by @pixelzoom)
• [x] ohms-law @pixelzoom
• [x] pendulum-lab
• [x] plinko-probability
• [x] projectile-motion
• [x] protein-synthesis @samreid (done by @pixelzoom)
• [x] resistance-in-a-wire @pixelzoom
• [x] rutherford-scattering
• [x] seasons @samreid (done by @pixelzoom)
• [x] states-of-matter-basics @jbphet (done by @pixelzoom)
• [ ] sugar-and-salt-solutions @samreid
• [ ] under-pressure @samreid

[jonathanolson]

There are some cases in Molecule Shapes where it would be nice to allow ignoring a prefix, like for 'control.showLonePairs', I had showLonePairsString (ignoring the control). controlShowLonePairsString (and the equivalents are a bit harder to read).

I'm moving over the usages to match the linter, but just thought I'd mention as an improvement.

[pixelzoom]

@jonathanolson wrote:

There are some cases in Molecule Shapes where it would be nice to allow ignoring a prefix

When we were discussing this, I mentioned that case in https://github.com/phetsims/chipper/issues/396#issuecomment-152583394, and asked for input from other developers. But I'm afraid that train has left the station, and I've already changed numerous cases just like this. So I think we should stick with the "no exceptions" rule.

[jonathanolson]

So I think we should stick with the "no exceptions" rule.

Ok, sounds good. Committed above, continuing for other sims.

[samreid]

I should also mention that if/when exceptions become necessary, they can be applied with

// eslint-disable-line [rule-name]

[jonathanolson]

I've updated my associated sims and a few older easy ones.

[pixelzoom]

I knocked off rutherford-scattering.

[pixelzoom]

At developer meeting, let's put names next to remaining repos in https://github.com/phetsims/chipper/issues/396#issuecomment-154550443.

[pixelzoom]

@jessegreenberg capacitor-lab-basics is still failing.

[jessegreenberg]

Thanks @pixelzoom, just pushed a fix.

[pixelzoom]

@jbphet and @samreid, I took care of a number of your sims, see check list.

[pixelzoom]

I created individual issues for the 6 remaining sims that are failing ESLint.

Closing this issue.