From 4c34c63c34f04bfb1646ee2350a51cf2879e49b8 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Thu, 7 Feb 2019 19:09:16 -0500 Subject: [PATCH 1/3] Blocks: Embed: Avoid unneccessary sprintf --- packages/block-library/src/embed/settings.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/block-library/src/embed/settings.js b/packages/block-library/src/embed/settings.js index d42a406b608ac..7995939c325a3 100644 --- a/packages/block-library/src/embed/settings.js +++ b/packages/block-library/src/embed/settings.js @@ -11,7 +11,7 @@ import classnames from 'classnames/dedupe'; /** * WordPress dependencies */ -import { __, sprintf } from '@wordpress/i18n'; +import { __ } from '@wordpress/i18n'; import { compose } from '@wordpress/compose'; import { RichText } from '@wordpress/editor'; import { withSelect, withDispatch } from '@wordpress/data'; @@ -38,8 +38,7 @@ const embedAttributes = { }; export function getEmbedBlockSettings( { title, description, icon, category = 'embed', transforms, keywords = [], supports = {}, responsive = true } ) { - // translators: %s: Name of service (e.g. VideoPress, YouTube) - const blockDescription = description || sprintf( __( 'Add a block that displays content pulled from other sites, like Twitter, Instagram or YouTube.' ), title ); + const blockDescription = description || __( 'Add a block that displays content pulled from other sites, like Twitter, Instagram or YouTube.' ); const edit = getEmbedEditComponent( title, icon, responsive ); return { title, From 1d1d5de025bdb5eed526859884d063bed8b507f7 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Thu, 7 Feb 2019 19:09:32 -0500 Subject: [PATCH 2/3] ESLint Plugin: Add rule valid-sprintf --- packages/eslint-plugin/CHANGELOG.md | 1 + packages/eslint-plugin/README.md | 1 + packages/eslint-plugin/configs/custom.js | 1 + .../eslint-plugin/docs/rules/valid-sprintf.md | 28 ++++ .../rules/__tests__/valid-sprintf.js | 75 +++++++++ packages/eslint-plugin/rules/valid-sprintf.js | 150 ++++++++++++++++++ 6 files changed, 256 insertions(+) create mode 100644 packages/eslint-plugin/docs/rules/valid-sprintf.md create mode 100644 packages/eslint-plugin/rules/__tests__/valid-sprintf.js create mode 100644 packages/eslint-plugin/rules/valid-sprintf.js diff --git a/packages/eslint-plugin/CHANGELOG.md b/packages/eslint-plugin/CHANGELOG.md index 9562f47730d07..daf663e8157e9 100644 --- a/packages/eslint-plugin/CHANGELOG.md +++ b/packages/eslint-plugin/CHANGELOG.md @@ -8,6 +8,7 @@ - New Rule: [`@wordpress/no-unused-vars-before-return`](https://github.com/WordPress/gutenberg/blob/master/packages/eslint-plugin/docs/rules/no-unused-vars-before-return.md) - New Rule: [`@wordpress/dependency-group`](https://github.com/WordPress/gutenberg/blob/master/packages/eslint-plugin/docs/rules/dependency-group.md) +- New Rule: [`@wordpress/valid-sprintf`](https://github.com/WordPress/gutenberg/blob/master/packages/eslint-plugin/docs/rules/valid-sprintf.md) ## 1.0.0 (2018-12-12) diff --git a/packages/eslint-plugin/README.md b/packages/eslint-plugin/README.md index 5e21e8fc99463..2af095ba3afec 100644 --- a/packages/eslint-plugin/README.md +++ b/packages/eslint-plugin/README.md @@ -51,6 +51,7 @@ Rule|Description ---|--- [no-unused-vars-before-return](/packages/eslint-plugin/docs/rules/no-unused-vars-before-return.md)|Disallow assigning variable values if unused before a return [dependency-group](/packages/eslint-plugin/docs/rules/dependency-group.md)|Enforce dependencies docblocks formatting +[valid-sprintf](/packages/eslint-plugin/docs/rules/valid-sprintf.md)|Disallow assigning variable values if unused before a return ### Legacy diff --git a/packages/eslint-plugin/configs/custom.js b/packages/eslint-plugin/configs/custom.js index 9eb42283099b0..7d79a3e055b18 100644 --- a/packages/eslint-plugin/configs/custom.js +++ b/packages/eslint-plugin/configs/custom.js @@ -5,6 +5,7 @@ module.exports = { rules: { '@wordpress/dependency-group': 'error', '@wordpress/no-unused-vars-before-return': 'error', + '@wordpress/valid-sprintf': 'error', 'no-restricted-syntax': [ 'error', { diff --git a/packages/eslint-plugin/docs/rules/valid-sprintf.md b/packages/eslint-plugin/docs/rules/valid-sprintf.md new file mode 100644 index 0000000000000..0ff0ccd67431f --- /dev/null +++ b/packages/eslint-plugin/docs/rules/valid-sprintf.md @@ -0,0 +1,28 @@ +# Enforce valid sprintf usage (valid-sprintf) + +[`sprintf`](https://github.com/WordPress/gutenberg/blob/master/packages/i18n/README.md#api) must be called with a valid format string with at least one placeholder, and with a valid set of placeholder substitute values. + +## Rule details + +Examples of **incorrect** code for this rule: + +```js +sprintf(); +sprintf( '%s' ); +sprintf( 1, 'substitute' ); +sprintf( [], 'substitute' ); +sprintf( '%%', 'substitute' ); +sprintf( __( '%%' ), 'substitute' ); +sprintf( _n( '%s', '' ), 'substitute' ); +sprintf( _n( '%s', '%s %s' ), 'substitute' ); +``` + +Examples of **correct** code for this rule: + +```js +sprintf( '%s', 'substitute' ); +sprintf( __( '%s' ), 'substitute' ); +sprintf( _x( '%s' ), 'substitute' ); +sprintf( _n( '%s', '%s' ), 'substitute' ); +sprintf( _nx( '%s', '%s' ), 'substitute' ); +``` diff --git a/packages/eslint-plugin/rules/__tests__/valid-sprintf.js b/packages/eslint-plugin/rules/__tests__/valid-sprintf.js new file mode 100644 index 0000000000000..06664969d8148 --- /dev/null +++ b/packages/eslint-plugin/rules/__tests__/valid-sprintf.js @@ -0,0 +1,75 @@ +/** + * External dependencies + */ +import { RuleTester } from 'eslint'; + +/** + * Internal dependencies + */ +import rule from '../valid-sprintf'; + +const ruleTester = new RuleTester( { + parserOptions: { + ecmaVersion: 6, + }, +} ); + +ruleTester.run( 'valid-sprintf', rule, { + valid: [ + { + code: `sprintf( '%s', 'substitute' )`, + }, + { + code: `sprintf( __( '%s' ), 'substitute' )`, + }, + { + code: `sprintf( _x( '%s' ), 'substitute' )`, + }, + { + code: `sprintf( _n( '%s', '%s' ), 'substitute' )`, + }, + { + code: `sprintf( _nx( '%s', '%s' ), 'substitute' )`, + }, + { + code: `var getValue = () => ''; sprintf( getValue(), 'substitute' )`, + }, + { + code: `var value = ''; sprintf( value, 'substitute' )`, + }, + ], + invalid: [ + { + code: `sprintf()`, + errors: [ { message: 'sprintf must be called with a format string' } ], + }, + { + code: `sprintf( '%s' )`, + errors: [ { message: 'sprintf must be called with placeholder value argument(s)' } ], + }, + { + code: `sprintf( 1, 'substitute' )`, + errors: [ { message: 'sprintf must be called with a valid format string' } ], + }, + { + code: `sprintf( [], 'substitute' )`, + errors: [ { message: 'sprintf must be called with a valid format string' } ], + }, + { + code: `sprintf( '%%', 'substitute' )`, + errors: [ { message: 'sprintf format string must contain at least one placeholder' } ], + }, + { + code: `sprintf( __( '%%' ), 'substitute' )`, + errors: [ { message: 'sprintf format string must contain at least one placeholder' } ], + }, + { + code: `sprintf( _n( '%s', '' ), 'substitute' )`, + errors: [ { message: 'sprintf format string options must have the same number of placeholders' } ], + }, + { + code: `sprintf( _n( '%s', '%s %s' ), 'substitute' )`, + errors: [ { message: 'sprintf format string options must have the same number of placeholders' } ], + }, + ], +} ); diff --git a/packages/eslint-plugin/rules/valid-sprintf.js b/packages/eslint-plugin/rules/valid-sprintf.js new file mode 100644 index 0000000000000..73101d12b7d67 --- /dev/null +++ b/packages/eslint-plugin/rules/valid-sprintf.js @@ -0,0 +1,150 @@ +/** + * Regular expression matching the presence of a printf format string + * placeholder. This naive pattern which does not validate the format. + * + * @type {RegExp} + */ +const REGEXP_PLACEHOLDER = /%[^%]/g; + +/** + * Given a function name and array of argument Node values, returns all + * possible string results from the corresponding translate function, or + * undefined if the function is not a translate function. + * + * @param {string} functionName Function name. + * @param {espree.Node[]} args Espree argument Node objects. + * + * @return {?Array} All possible translate function string results. + */ +function getTranslateStrings( functionName, args ) { + switch ( functionName ) { + case '__': + case '_x': + args = args.slice( 0, 1 ); + break; + + case '_n': + case '_nx': + args = args.slice( 0, 2 ); + break; + + default: + return; + } + + return args + .filter( ( arg ) => arg.type === 'Literal' ) + .map( ( arg ) => arg.value ); +} + +module.exports = { + meta: { + type: 'problem', + schema: [], + }, + create( context ) { + return { + CallExpression( node ) { + const { callee, arguments: args } = node; + if ( callee.name !== 'sprintf' ) { + return; + } + + if ( ! args.length ) { + context.report( + node, + 'sprintf must be called with a format string' + ); + return; + } + + if ( args.length < 2 ) { + context.report( + node, + 'sprintf must be called with placeholder value argument(s)' + ); + return; + } + + let candidates; + switch ( args[ 0 ].type ) { + case 'Literal': + candidates = [ args[ 0 ].value ].filter( ( arg ) => { + // Since a Literal may be a number, verify the + // value is a string. + return typeof arg === 'string'; + } ); + break; + + case 'CallExpression': + // All possible options (arguments) from a translate + // function must be valid. + candidates = getTranslateStrings( + args[ 0 ].callee.name, + args[ 0 ].arguments + ); + + // An unknown function call may produce a valid string + // value. Ideally its result is verified, but this is + // not straight-forward to implement. Thus, bail. + if ( candidates === undefined ) { + return; + } + + break; + + case 'Identifier': + // Identifiers may refer to a valid string variable. + // Ideally its reference value is verified, but this is + // not straight-forward to implement. Thus, bail. + return; + + default: + candidates = []; + } + + if ( ! candidates.length ) { + context.report( + node, + 'sprintf must be called with a valid format string' + ); + return; + } + + let numPlaceholders; + for ( let i = 0; i < candidates.length; i++ ) { + const match = candidates[ i ].match( REGEXP_PLACEHOLDER ); + + // Prioritize placeholder number consistency over matching + // placeholder, since it's a more common error to omit a + // placeholder from the singular form of pluralization. + if ( + numPlaceholders !== undefined && + ( ! match || numPlaceholders !== match.length ) + ) { + context.report( + node, + 'sprintf format string options must have the same number of placeholders' + ); + return; + } + + if ( ! match ) { + context.report( + node, + 'sprintf format string must contain at least one placeholder' + ); + return; + } + + if ( numPlaceholders === undefined ) { + // Track the number of placeholders discovered in the + // string to verify that all other candidate options + // have the same number. + numPlaceholders = match.length; + } + } + }, + }; + }, +}; From afe9838fc0a1ff9b0aa6e25756f33f841c134b42 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Thu, 7 Feb 2019 19:41:52 -0500 Subject: [PATCH 3/3] i18n: Disable valid-sprintf for intentional error test case --- packages/i18n/src/test/index.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/i18n/src/test/index.js b/packages/i18n/src/test/index.js index c79d24fb43bc2..19d398daa47ef 100644 --- a/packages/i18n/src/test/index.js +++ b/packages/i18n/src/test/index.js @@ -68,6 +68,8 @@ describe( 'i18n', () => { describe( 'sprintf()', () => { it( 'absorbs errors', () => { + // Disable reason: Failing case is the purpose of the test. + // eslint-disable-next-line @wordpress/valid-sprintf const result = sprintf( 'Hello %(placeholder-not-provided)s' ); expect( console ).toHaveErrored();