Skip to content

ESLint Plugin: Add rule valid-sprintf#13756

Merged
aduth merged 3 commits intomasterfrom
fix/embed-description-placeholder
Feb 11, 2019
Merged

ESLint Plugin: Add rule valid-sprintf#13756
aduth merged 3 commits intomasterfrom
fix/embed-description-placeholder

Conversation

@aduth
Copy link
Copy Markdown
Member

@aduth aduth commented Feb 8, 2019

Related: #6624 (comment)

This pull request seeks to introduce a new ESLint rule to help protect against invalid use of sprintf.

Specifically, it enforces:

  • That it's called with a format string
  • That it's called with 1 or more placeholder substitute values
  • That the format string contains at least one placeholder
  • That each format string option (if using _n or _nx) contain an identical number of placeholders (a common error is forming a string like sprintf( _n( 'One cat', '%d cats', num ) ))

It does not (yet) enforce:

  • That the placeholder formats are valid
  • That the number of placeholders matches the number of arguments passed (this becomes tricky, especially with named placeholders and re-use of positional arguments e.g. sprintf( '%1$s %1$s', '1' ) is valid)

Testing instructions:

Verify that the description for the Embed block shows as expected:

Add a block that displays content pulled from other sites, like Twitter, Instagram or YouTube.

Ensure tests pass:

npm test

@aduth aduth added Internationalization (i18n) Issues or PRs related to internationalization efforts [Tool] ESLint plugin /packages/eslint-plugin labels Feb 8, 2019
@aduth aduth requested a review from swissspidy February 8, 2019 00:17
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, so an example of something that got picked up by the new rule ;) nice.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pull request started as a very roundabout way of addressing this specific issue 😆

@gziolo gziolo added this to the 5.1 (Gutenberg) milestone Feb 8, 2019
@aduth aduth force-pushed the fix/embed-description-placeholder branch from 7abe8bb to afe9838 Compare February 8, 2019 19:54
@aduth aduth requested a review from oandregal as a code owner February 8, 2019 19:54
@aduth aduth merged commit 52e8384 into master Feb 11, 2019
@aduth aduth deleted the fix/embed-description-placeholder branch February 11, 2019 13:48
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
* Blocks: Embed: Avoid unneccessary sprintf

* ESLint Plugin: Add rule valid-sprintf

* i18n: Disable valid-sprintf for intentional error test case
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
* Blocks: Embed: Avoid unneccessary sprintf

* ESLint Plugin: Add rule valid-sprintf

* i18n: Disable valid-sprintf for intentional error test case
---|---
[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
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This description is not correct 😅 #14666

This was referenced Apr 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Internationalization (i18n) Issues or PRs related to internationalization efforts [Tool] ESLint plugin /packages/eslint-plugin

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants