From 65d0af448cb1516783db9a46ed0af0ed0ecba3f7 Mon Sep 17 00:00:00 2001 From: zepumph Date: Thu, 7 Nov 2019 11:58:58 -0900 Subject: [PATCH] report lines for everything by default, use codeTokens only for "code only", https://github.com/phetsims/chipper/issues/791 --- eslint/rules/bad-sim-text.js | 18 +++++----- eslint/rules/bad-text.js | 7 ++-- eslint/rules/getBadTextTester.js | 62 ++++++++++++++++---------------- 3 files changed, 45 insertions(+), 42 deletions(-) diff --git a/eslint/rules/bad-sim-text.js b/eslint/rules/bad-sim-text.js index 081a9a11a..aa0d9abd3 100644 --- a/eslint/rules/bad-sim-text.js +++ b/eslint/rules/bad-sim-text.js @@ -20,22 +20,22 @@ module.exports = function( context ) { const forbiddenTextObjects = [ // babel doesn't support compiling static getters, see https://github.com/phetsims/tasks/issues/983 - { id: ' static get ', codeOnly: true, codeTokens: [ 'static', 'get' ] }, + { id: ' static get ', codeTokens: [ 'static', 'get' ] }, // should be using dot.Util.roundSymmetric, Math.round does not treat positive and negative numbers // symmetrically see https://github.com/phetsims/dot/issues/35#issuecomment-113587879 - { id: 'Math.round(', codeOnly: true, codeTokens: [ 'Math', '.', 'round', '(' ] }, + { id: 'Math.round(', codeTokens: [ 'Math', '.', 'round', '(' ] }, // should be using `phet.joist.random` - { id: 'Math.random()', codeOnly: true, codeTokens: [ 'Math', '.', 'random', '(', ')' ] }, - { id: '_.shuffle(', codeOnly: true, codeTokens: [ '_', '.', 'shuffle', '(' ] }, - { id: '_.sample(', codeOnly: true, codeTokens: [ '_', '.', 'sample', '(' ] }, - { id: '_.random(', codeOnly: true, codeTokens: [ '_', '.', 'random', '(' ] }, - { id: 'new Random()', codeOnly: true, codeTokens: [ 'new', 'Random', '(', ')' ] }, + { id: 'Math.random()', codeTokens: [ 'Math', '.', 'random', '(', ')' ] }, + { id: '_.shuffle(', codeTokens: [ '_', '.', 'shuffle', '(' ] }, + { id: '_.sample(', codeTokens: [ '_', '.', 'sample', '(' ] }, + { id: '_.random(', codeTokens: [ '_', '.', 'random', '(' ] }, + { id: 'new Random()', codeTokens: [ 'new', 'Random', '(', ')' ] }, // IE doesn't support: - { id: 'Number.parseInt(', codeOnly: true, codeTokens: [ 'Number', '.', 'parseInt', '(' ] }, - { id: 'Array.prototype.find', codeOnly: true, codeTokens: [ 'Array', '.', 'prototype', '.', 'find' ] } + { id: 'Number.parseInt(', codeTokens: [ 'Number', '.', 'parseInt', '(' ] }, + { id: 'Array.prototype.find', codeTokens: [ 'Array', '.', 'prototype', '.', 'find' ] } // DOT/Util.toFixed or DOT/Util.toFixedNumber should be used instead of toFixed. // JavaScript's toFixed is notoriously buggy. Behavior differs depending on browser, diff --git a/eslint/rules/bad-text.js b/eslint/rules/bad-text.js index 33b442c73..face403c4 100644 --- a/eslint/rules/bad-text.js +++ b/eslint/rules/bad-text.js @@ -45,15 +45,16 @@ module.exports = function( context ) { // For phet-io use PHET_IO in constants 'PHETIO', + 'PHET-IO', 'Phet-iO', ' Phet ', 'phetio element', // use "phet-io element" or "PhET-iO element" 'Phet-iO', - '@return ' + '@return ', - // TODO: uncomment once ESP(B) lint errors fixed, see https://github.com/phetsims/energy-skate-park/issues/140 - // { id: ' => { return ', global: true } // if on a one line arrow function returning something, prefer instead `() => theReturn`, see https://github.com/phetsims/chipper/issues/790 + // if on a one line arrow function returning something, prefer instead `() => theReturn`, see https://github.com/phetsims/chipper/issues/790 + ' => { return ' ]; return { diff --git a/eslint/rules/getBadTextTester.js b/eslint/rules/getBadTextTester.js index 17434d306..3a0782d56 100644 --- a/eslint/rules/getBadTextTester.js +++ b/eslint/rules/getBadTextTester.js @@ -24,7 +24,7 @@ module.exports = ( badTexts, context ) => { return node => { const sourceCode = context.getSourceCode(); const codeTokens = sourceCode.getTokens( node ); - const commentTokens = sourceCode.getAllComments(); + const codeLines = sourceCode.lines; const text = sourceCode.text; /** @@ -33,45 +33,50 @@ module.exports = ( badTexts, context ) => { */ const testBadText = forbiddenText => { - // no need to iterate through tokens if it isn't anywhere in the source code + // no need to iterate through lines if the bad text isn't anywhere in the source code if ( text.indexOf( forbiddenText.id ) >= 0 ) { - // If marked as global, then report the global failure based on the source code indexOf check - if ( forbiddenText.global ) { - context.report( { - node: node, - message: 'File contains bad text: \'' + forbiddenText.id + '\'' - } ); + // If codeTokens are provided, only test this bad text in code, and not anywhere else. + if ( forbiddenText.codeTokens ) { + testCodeTokens( context, codeTokens, forbiddenText ); } else { - // search through the tokenized code for the forbidden code tokens, like [ 'Math', '.', 'round' ], - testCodeTokens( context, codeTokens, forbiddenText ); - - // look through comments - !forbiddenText.codeOnly && commentTokens.forEach( token => { - if ( token.value.indexOf( forbiddenText.id ) >= 0 ) { - context.report( { - loc: token.loc.start, - message: `bad comment text: "${forbiddenText.id}"` - } ); - } - } ); - // TODO: support REGEX // if ( forbiddenText.regex instanceof RegExp && forbiddenText.regex.test( token.value ) ) { // failedText = forbiddenText.id; // } + + // test each line for the presence of the bad text + for ( let i = 0; i < codeLines.length; i++ ) { + const columnIndex = codeLines[ i ].indexOf( forbiddenText.id ); + if ( columnIndex >= 0 ) { + + // lines are 1 based, codeLines array is 0 based + const badLine = i + 1; + + // esprima Token loc object, see https://esprima.readthedocs.io/en/latest/lexical-analysis.html + const loc = { + start: { line: badLine, column: columnIndex }, + end: { line: badLine, column: columnIndex + forbiddenText.id.length } + }; + + context.report( { + node: node, + loc: loc, + message: 'Line contains bad text: \'' + forbiddenText.id + '\'' + } ); + } + } } } }; badTexts.forEach( badText => { if ( typeof badText === 'string' ) { - badText = { id: badText, codeTokens: [ badText ] }; + badText = { id: badText }; } assert( typeof badText.id === 'string', 'id required' ); - assert( Array.isArray( badText.codeTokens ) || badText.global, 'code tokens or global flag expected' ); testBadText( badText ); } ); }; @@ -81,13 +86,10 @@ module.exports = ( badTexts, context ) => { * @property {string} id - the "string-form" id of the bad text. should occur in the source code. Also what is * displayed on error. Used when checking for bad text in comments. * @property {Array.} [codeTokens] - a list of the tokenized, ordered code sections that make up the bad text - * within the javascript code (not used for checking comments). If there - * is only one codeToken, then it will also be checked as a substring of each - * code tokens. Required unless specifying "global". - * @property {boolean} [codeOnly] - if true, this object will not be checked for in comments, only in code. - * @property {boolean} [global] - if true, then ignore comment and code token checks, and just error out based on the - * presence of the `id` in the source code for the file. If provided, then codeTokens - * is not needed + * within the javascript code (not used for checking comments). If there + * is only one codeToken, then it will also be checked as a substring of each + * code tokens. Required unless specifying "global". If this is provided, + * then the bad text will only be checked in code, and not via each line. */ };