Extend valid-sprintf ESLint rule to catch placeholders that sh… (#20574)

* Add new ESLint rule to validate text domains * Enforce `@wordpress/valid-text-domain` rule for Gutenberg code base * Use messageId and make fixable * Add new no-missing-translator-comments rule * Enforce `@wordpress/no-missing-translator-comments` rule for Gutenberg code base * Add docs * Implement feedback from code review * Rename rule names * Simplify getting previousArg * Extract and document utils * Combine comments * Derive allowDefault from allowedTextDomains * Break early for line number mismatches Co-Authored-By: Andrew Duthie <[email protected]> * Support `i18n.*` usage in new rules * Add new i18n-no-variables rule * Add new i18n-ellipsis rule * Add new i18n-no-placeholders-only rule * Add new i18n-no-collapsible-whitespace rule * Use messageId in valid-sprintf rule * Fix code base after change * Catch mix of ordered and non-ordered placeholders in valid-sprintf rule * Mark as breaking change in the readme * Support `i18n.*` usage in `valid-sprintf` rule * Disable i18n-no-collapsible-whitespace rule for now * Remove unneded capture group * Use Set for list of translation functions * Move const to top scope * Coding standards in code examples * Refactor utils to make code more DRY * Coding standards in test code * Remove now unneeded no-restricted-syntax config * Add i18n rules to new i18n config * Mark new ruleset as breaking change * Update docs * Fix tests * Rename argument to allowedTextDomain and allow strings and arrays * Fix case when using object spread * Apply changes from code review * Rename messageId to make it more clear * Update regex to properly allow named arguments which are supported * Update i18n-no-placeholders-only rule Co-authored-by: Andrew Duthie <[email protected]>
WordPress · Apr 3, 2020 · 969bbe9 · 969bbe9
1 parent 9a6da4a
commit 969bbe9
Show file tree

Hide file tree

Showing 10 changed files with 219 additions and 94 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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;