diff --git a/packages/blocks/src/api/utils.js b/packages/blocks/src/api/utils.js index 11b333d8bdef2..dbf1dc79049b9 100644 --- a/packages/blocks/src/api/utils.js +++ b/packages/blocks/src/api/utils.js @@ -184,7 +184,7 @@ export function getAccessibleBlockLabel( if ( hasPosition && direction === 'vertical' ) { if ( hasLabel ) { return sprintf( - /* translators: accessibility text. %1: The block title, %2: The block row number, %3: The block label.. */ + /* translators: accessibility text. 1: The block title. 2: The block row number. 3: The block label.. */ __( '%1$s Block. Row %2$d. %3$s' ), title, position, @@ -193,15 +193,15 @@ export function getAccessibleBlockLabel( } return sprintf( - /* translators: accessibility text. %s: The block title, %d The block row number. */ - __( '%s Block. Row %d' ), + /* translators: accessibility text. 1: The block title. 2: The block row number. */ + __( '%1$s Block. Row %2$d' ), title, position ); } else if ( hasPosition && direction === 'horizontal' ) { if ( hasLabel ) { return sprintf( - /* translators: accessibility text. %1: The block title, %2: The block column number, %3: The block label.. */ + /* translators: accessibility text. 1: The block title. 2: The block column number. 3: The block label.. */ __( '%1$s Block. Column %2$d. %3$s' ), title, position, @@ -210,8 +210,8 @@ export function getAccessibleBlockLabel( } return sprintf( - /* translators: accessibility text. %s: The block title, %d The block column number. */ - __( '%s Block. Column %d' ), + /* translators: accessibility text. 1: The block title. 2: The block column number. */ + __( '%1$s Block. Column %2$d' ), title, position ); diff --git a/packages/eslint-plugin/CHANGELOG.md b/packages/eslint-plugin/CHANGELOG.md index df3ba08a95775..f4a0b1ada1a8e 100644 --- a/packages/eslint-plugin/CHANGELOG.md +++ b/packages/eslint-plugin/CHANGELOG.md @@ -13,7 +13,12 @@ ### Breaking Changes - There is a new `i18n` ruleset that includes all i18n-related rules and is included in the `recommended` ruleset. -- The `valid-sprintf` rule has been moved from the `custom` ruleset to the `i18n` ruleset. +- The `@wordpress/valid-sprintf` rule has been moved from the `custom` ruleset to the `i18n` ruleset. +- The `@wordpress/valid-sprintf` rule now recognizes mix of ordered and non-ordered placeholders. + +### Bug Fix + +- The `@wordpress/valid-sprintf` rule now detects usage of `sprintf` via `i18n.sprintf` (e.g. when using `import * as i18n from '@wordpress/i18n'`). ## 4.0.0 (2020-02-10) diff --git a/packages/eslint-plugin/rules/__tests__/i18n-no-placeholders-only.js b/packages/eslint-plugin/rules/__tests__/i18n-no-placeholders-only.js index 747e380e67f7e..7743798805267 100644 --- a/packages/eslint-plugin/rules/__tests__/i18n-no-placeholders-only.js +++ b/packages/eslint-plugin/rules/__tests__/i18n-no-placeholders-only.js @@ -19,6 +19,9 @@ ruleTester.run( 'i18n-no-placeholders-only', rule, { { code: `__( 'Hello %s' )`, }, + { + code: `i18n.__( 'Hello %s' )`, + }, { code: `__( '%d%%' )`, }, @@ -32,11 +35,10 @@ ruleTester.run( 'i18n-no-placeholders-only', rule, { code: `__( '%s%s' )`, errors: [ { messageId: 'noPlaceholdersOnly' } ], }, - // @todo: Update placeholder regex, see https://github.com/WordPress/gutenberg/pull/20574. - /*{ + { code: `_x( '%1$s' )`, errors: [ { messageId: 'noPlaceholdersOnly' } ], - },*/ + }, { code: `_n( '%s', '%s', number)`, errors: [ diff --git a/packages/eslint-plugin/rules/__tests__/valid-sprintf.js b/packages/eslint-plugin/rules/__tests__/valid-sprintf.js index 119653220b3c0..9b2b7de255d47 100644 --- a/packages/eslint-plugin/rules/__tests__/valid-sprintf.js +++ b/packages/eslint-plugin/rules/__tests__/valid-sprintf.js @@ -19,6 +19,9 @@ ruleTester.run( 'valid-sprintf', rule, { { code: `sprintf( '%s', 'substitute' )`, }, + { + code: `sprintf( '%1$d%%', 500 )`, + }, { code: `sprintf( __( '%s' ), 'substitute' )`, }, @@ -37,76 +40,92 @@ ruleTester.run( 'valid-sprintf', rule, { { code: `var value = ''; sprintf( value, 'substitute' )`, }, + { + code: ` +sprintf( + /* translators: 1: number of blocks. 2: average rating. */ + _n( + 'This author has %1$d block, with an average rating of %2$d.', + 'This author has %1$d blocks, with an average rating of %2$d.', + authorBlockCount + ), + authorBlockCount, + authorBlockRating +);`, + }, + { + code: `i18n.sprintf( '%s', 'substitute' )`, + }, + { + code: `i18n.sprintf( i18n.__( '%s' ), 'substitute' )`, + }, + { + code: `sprintf( ...args )`, + }, + { + code: `sprintf( '%1$s %2$s', 'foo', 'bar' )`, + }, + { + code: `sprintf( '%(greeting)s', 'Hello' )`, + }, + { + code: `sprintf( '%(greeting)s %(toWhom)s', 'Hello', 'World' )`, + }, ], invalid: [ { code: `sprintf()`, - errors: [ - { message: 'sprintf must be called with a format string' }, - ], + errors: [ { messageId: 'noFormatString' } ], }, { code: `sprintf( '%s' )`, - errors: [ - { - message: - 'sprintf must be called with placeholder value argument(s)', - }, - ], + errors: [ { messageId: 'noPlaceholderArgs' } ], }, { code: `sprintf( 1, 'substitute' )`, - errors: [ - { - message: - 'sprintf must be called with a valid format string', - }, - ], + errors: [ { messageId: 'invalidFormatString' } ], }, { code: `sprintf( [], 'substitute' )`, - errors: [ - { - message: - 'sprintf must be called with a valid format string', - }, - ], + errors: [ { messageId: 'invalidFormatString' } ], }, { code: `sprintf( '%%', 'substitute' )`, - errors: [ - { - message: - 'sprintf format string must contain at least one placeholder', - }, - ], + errors: [ { messageId: 'noPlaceholders' } ], }, { code: `sprintf( __( '%%' ), 'substitute' )`, - errors: [ - { - message: - 'sprintf format string must contain at least one placeholder', - }, - ], + errors: [ { messageId: 'noPlaceholders' } ], }, { code: `sprintf( _n( '%s', '' ), 'substitute' )`, - errors: [ - { - message: - 'sprintf format string options must have the same number of placeholders', - }, - ], + errors: [ { messageId: 'placeholderMismatch' } ], }, { code: `sprintf( _n( '%s', '%s %s' ), 'substitute' )`, - errors: [ - { - message: - 'sprintf format string options must have the same number of placeholders', - }, - ], + errors: [ { messageId: 'placeholderMismatch' } ], + }, + { + code: ` +sprintf( + /* translators: 1: number of blocks. 2: average rating. */ + _n( + 'This author has %d block, with an average rating of %d.', + 'This author has %d blocks, with an average rating of %d.', + authorBlockCount + ), + authorBlockCount, + authorBlockRating +);`, + errors: [ { messageId: 'noOrderedPlaceholders' } ], + }, + { + code: `i18n.sprintf()`, + errors: [ { messageId: 'noFormatString' } ], + }, + { + code: `i18n.sprintf( i18n.__( '%%' ), 'substitute' )`, + errors: [ { messageId: 'noPlaceholders' } ], }, ], } ); diff --git a/packages/eslint-plugin/rules/i18n-no-placeholders-only.js b/packages/eslint-plugin/rules/i18n-no-placeholders-only.js index 796e42c0b0dc1..3cc2b3dc34fb0 100644 --- a/packages/eslint-plugin/rules/i18n-no-placeholders-only.js +++ b/packages/eslint-plugin/rules/i18n-no-placeholders-only.js @@ -3,7 +3,7 @@ */ const { TRANSLATION_FUNCTIONS, - REGEXP_PLACEHOLDER, + REGEXP_SPRINTF_PLACEHOLDER, getTextContentFromNode, getTranslateFunctionName, getTranslateFunctionArgs, @@ -40,10 +40,9 @@ module.exports = { continue; } - const modifiedString = argumentString.replace( - REGEXP_PLACEHOLDER, - '' - ); + const modifiedString = argumentString + .replace( /%%/g, 'VALID_ESCAPED_PERCENTAGE_SIGN' ) + .replace( REGEXP_SPRINTF_PLACEHOLDER, '' ); if ( modifiedString.length > 0 ) { continue; diff --git a/packages/eslint-plugin/rules/i18n-translator-comments.js b/packages/eslint-plugin/rules/i18n-translator-comments.js index 8b01ba009ffed..86d57449dc054 100644 --- a/packages/eslint-plugin/rules/i18n-translator-comments.js +++ b/packages/eslint-plugin/rules/i18n-translator-comments.js @@ -3,7 +3,7 @@ */ const { TRANSLATION_FUNCTIONS, - REGEXP_PLACEHOLDER, + REGEXP_SPRINTF_PLACEHOLDER, getTranslateFunctionName, getTranslateFunctionArgs, getTextContentFromNode, @@ -45,10 +45,10 @@ module.exports = { } const hasPlaceholders = candidates.some( ( candidate ) => - REGEXP_PLACEHOLDER.test( candidate ) + REGEXP_SPRINTF_PLACEHOLDER.test( candidate ) ); // See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp/test#Using_test()_on_a_regex_with_the_global_flag. - REGEXP_PLACEHOLDER.lastIndex = 0; + REGEXP_SPRINTF_PLACEHOLDER.lastIndex = 0; if ( ! hasPlaceholders ) { return; diff --git a/packages/eslint-plugin/rules/valid-sprintf.js b/packages/eslint-plugin/rules/valid-sprintf.js index b37cfa65f0a34..593ea11b564a0 100644 --- a/packages/eslint-plugin/rules/valid-sprintf.js +++ b/packages/eslint-plugin/rules/valid-sprintf.js @@ -2,7 +2,9 @@ * Internal dependencies */ const { - REGEXP_PLACEHOLDER, + REGEXP_SPRINTF_PLACEHOLDER, + REGEXP_SPRINTF_PLACEHOLDER_UNORDERED, + getTranslateFunctionName, getTranslateFunctionArgs, getTextContentFromNode, } = require( '../utils' ); @@ -11,28 +13,51 @@ module.exports = { meta: { type: 'problem', schema: [], + messages: { + noFormatString: 'sprintf must be called with a format string', + invalidFormatString: + 'sprintf must be called with a valid format string', + noPlaceholderArgs: + 'sprintf must be called with placeholder value argument(s)', + noPlaceholders: + 'sprintf format string must contain at least one placeholder', + placeholderMismatch: + 'sprintf format string options must have the same number of placeholders', + noOrderedPlaceholders: + 'Multiple sprintf placeholders should be ordered. Mix of ordered and non-ordered placeholders found.', + }, }, create( context ) { return { CallExpression( node ) { const { callee, arguments: args } = node; - if ( callee.name !== 'sprintf' ) { + + const functionName = + callee.property && callee.property.name + ? callee.property.name + : callee.name; + + if ( functionName !== 'sprintf' ) { return; } if ( ! args.length ) { - context.report( + context.report( { node, - 'sprintf must be called with a format string' - ); + messageId: 'noFormatString', + } ); return; } if ( args.length < 2 ) { - context.report( + if ( args[ 0 ].type === 'SpreadElement' ) { + return; + } + + context.report( { node, - 'sprintf must be called with placeholder value argument(s)' - ); + messageId: 'noPlaceholderArgs', + } ); return; } @@ -47,10 +72,14 @@ module.exports = { break; case 'CallExpression': + const argFunctionName = getTranslateFunctionName( + args[ 0 ].callee + ); + // All possible options (arguments) from a translate // function must be valid. candidates = getTranslateFunctionArgs( - args[ 0 ].callee.name, + argFunctionName, args[ 0 ].arguments, false ).map( getTextContentFromNode ); @@ -75,36 +104,61 @@ module.exports = { } if ( ! candidates.length ) { - context.report( + context.report( { node, - 'sprintf must be called with a valid format string' - ); + messageId: 'invalidFormatString', + } ); return; } let numPlaceholders; - for ( let i = 0; i < candidates.length; i++ ) { - const match = candidates[ i ].match( REGEXP_PLACEHOLDER ); + for ( const candidate of candidates ) { + const allMatches = candidate.match( + REGEXP_SPRINTF_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 ) + ( ! allMatches || + numPlaceholders !== allMatches.length ) ) { - context.report( + context.report( { node, - 'sprintf format string options must have the same number of placeholders' - ); + messageId: 'placeholderMismatch', + } ); return; } - if ( ! match ) { - context.report( + const unorderedMatches = candidate.match( + REGEXP_SPRINTF_PLACEHOLDER_UNORDERED + ); + + if ( + unorderedMatches && + allMatches && + unorderedMatches.length > 0 && + allMatches.length > 1 && + unorderedMatches.length !== allMatches.length + ) { + context.report( { node, - 'sprintf format string must contain at least one placeholder' - ); + messageId: 'noOrderedPlaceholders', + } ); + return; + } + + // Catch cases where a string only contains %% (escaped percentage sign). + if ( + ! allMatches || + ( allMatches.length === 1 && allMatches[ 0 ] === '%%' ) + ) { + context.report( { + node, + messageId: 'noPlaceholders', + } ); return; } @@ -112,7 +166,7 @@ module.exports = { // Track the number of placeholders discovered in the // string to verify that all other candidate options // have the same number. - numPlaceholders = match.length; + numPlaceholders = allMatches.length; } } }, diff --git a/packages/eslint-plugin/utils/constants.js b/packages/eslint-plugin/utils/constants.js index 5ef2980ed6255..66ba5b52b63a5 100644 --- a/packages/eslint-plugin/utils/constants.js +++ b/packages/eslint-plugin/utils/constants.js @@ -6,14 +6,55 @@ const TRANSLATION_FUNCTIONS = new Set( [ '__', '_x', '_n', '_nx' ] ); /** - * Regular expression matching the presence of a printf format string - * placeholder. This naive pattern which does not validate the format. + * Regular expression matching format placeholder syntax. + * + * The pattern for matching named arguments is a naive and incomplete matcher + * against valid JavaScript identifier names. + * + * via Mathias Bynens: + * + * >An identifier must start with $, _, or any character in the Unicode + * >categories “Uppercase letter (Lu)”, “Lowercase letter (Ll)”, “Titlecase + * >letter (Lt)”, “Modifier letter (Lm)”, “Other letter (Lo)”, or “Letter + * >number (Nl)”. + * > + * >The rest of the string can contain the same characters, plus any U+200C zero + * >width non-joiner characters, U+200D zero width joiner characters, and + * >characters in the Unicode categories “Non-spacing mark (Mn)”, “Spacing + * >combining mark (Mc)”, “Decimal digit number (Nd)”, or “Connector + * >punctuation (Pc)”. + * + * If browser support is constrained to those supporting ES2015, this could be + * made more accurate using the `u` flag: + * + * ``` + * /^[$_\p{L}\p{Nl}][$_\p{L}\p{Nl}\u200C\u200D\p{Mn}\p{Mc}\p{Nd}\p{Pc}]*$/u; + * ``` + * + * @see http://www.pixelbeat.org/programming/gcc/format_specs.html + * @see https://mathiasbynens.be/notes/javascript-identifiers#valid-identifier-names + * + * @type {RegExp} + */ +const REGEXP_SPRINTF_PLACEHOLDER = /%(((\d+)\$)|(\(([$_a-zA-Z][$_a-zA-Z0-9]*)\)))?[ +0#-]*\d*(\.(\d+|\*))?(ll|[lhqL])?([cduxXefgsp%])/g; +// ▲ ▲ ▲ ▲ ▲ ▲ ▲ type +// │ │ │ │ │ └ Length (unsupported) +// │ │ │ │ └ Precision / max width +// │ │ │ └ Min width (unsupported) +// │ │ └ Flags (unsupported) +// └ Index └ Name (for named arguments) + +/** + * "Unordered" means there's no position specifier: '%s', not '%2$s'. + * + * @see https://github.com/WordPress/WordPress-Coding-Standards/blob/2f927b0ba2bfcbffaa8f3251c086e109302d6622/WordPress/Sniffs/WP/I18nSniff.php#L62-L81 * * @type {RegExp} */ -const REGEXP_PLACEHOLDER = /%[^%]/g; +const REGEXP_SPRINTF_PLACEHOLDER_UNORDERED = /(?:(?