Extend valid-sprintf ESLint rule to catch placeholders that should be numbered

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
 		);

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)
 

packages/eslint-plugin/rules/__tests__/valid-sprintf.js

@@ -37,76 +37,83 @@ 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 )`,
+		},
 	],
 	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: 'noNumberedPlaceholders' } ],
+		},
+		{
+			code: `i18n.sprintf()`,
+			errors: [ { messageId: 'noFormatString' } ],
+		},
+		{
+			code: `i18n.sprintf( i18n.__( '%%' ), 'substitute' )`,
+			errors: [ { messageId: 'noPlaceholders' } ],
 		},
 	],
 } );

packages/eslint-plugin/rules/i18n-no-placeholders-only.js

@@ -3,7 +3,7 @@
  */
 const {
 	TRANSLATION_FUNCTIONS,
-	REGEXP_PLACEHOLDER,
+	REGEXP_SPRINTF_PLACEHOLDER,
 	getTextContentFromNode,
 	getTranslateFunctionName,
 	getTranslateFunctionArgs,
@@ -41,7 +41,7 @@ module.exports = {
 					}
 
 					const modifiedString = argumentString.replace(
-						REGEXP_PLACEHOLDER,
+						REGEXP_SPRINTF_PLACEHOLDER,
 						''
 					);
 

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;

packages/eslint-plugin/rules/valid-sprintf.js

@@ -2,7 +2,8 @@
  * Internal dependencies
  */
 const {
-	REGEXP_PLACEHOLDER,
+	REGEXP_SPRINTF_PLACEHOLDER,
+	REGEXP_SPRINTF_PLACEHOLDER_UNORDERED,
 	getTranslateFunctionArgs,
 	getTextContentFromNode,
 } = require( '../utils' );
@@ -11,28 +12,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',
+			noNumberedPlaceholders:
+				'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 +71,16 @@ module.exports = {
 						break;
 
 					case 'CallExpression':
+						const argFunctionName =
+							args[ 0 ].callee.property &&
+							args[ 0 ].callee.property.name
+								? args[ 0 ].callee.property.name
+								: args[ 0 ].callee.name;
+
 						// 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,44 +105,64 @@ 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
+					);
+					const unorderedMatches = candidate.match(
+						REGEXP_SPRINTF_PLACEHOLDER_UNORDERED
+					);
 
 					// 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( {
+							node,
+							messageId: 'placeholderMismatch',
+						} );
+						return;
+					}
+
+					if (
+						unorderedMatches &&
+						allMatches &&
+						unorderedMatches.length > 0 &&
+						allMatches.length > 1 &&
+						unorderedMatches.length !== allMatches.length
 					) {
-						context.report(
+						context.report( {
 							node,
-							'sprintf format string options must have the same number of placeholders'
-						);
+							messageId: 'noNumberedPlaceholders',
+						} );
 						return;
 					}
 
-					if ( ! match ) {
-						context.report(
+					if ( ! allMatches ) {
+						context.report( {
 							node,
-							'sprintf format string must contain at least one placeholder'
-						);
+							messageId: 'noPlaceholders',
+						} );
 						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;
+						numPlaceholders = allMatches.length;
 					}
 				}
 			},