Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ESLint Plugin: Add rule valid-sprintf #13756

Merged
merged 3 commits into from
Feb 11, 2019
Merged

Conversation

aduth
Copy link
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
@@ -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.' );
Copy link
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
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
@@ -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
Copy link
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