Skip to content

Add support for precision type placeholders to translator comments eslint #71145

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Aug 21, 2025
Merged

Add support for precision type placeholders to translator comments eslint #71145

merged 3 commits into from
Aug 21, 2025

Conversation

@USERSATOSHI
Copy link
Contributor

@USERSATOSHI USERSATOSHI commented Aug 8, 2025

What?

Addresses: comment #70458

This PR adds support for precision based placeholders to be allowed in translator comments by eslint.

This PR also fixes false positives like 6:00 AM by forcing a placeholder to have space after : to be counted as placeholder.

Why?

Currently eslint for translator comments flags cases that are supposed to be valid by counting words like 6:00 AM as placeholder due to : placement.

Alongside this, support for precision based placeholder was not available causing translators comments to fail for placeholders like %.*s, 1$.2f etc.

Example

  // translators: %s: a formatted time (e.g. 6:00AM).
  const test = __( 'Alarm is set for %s', 'some-pkg' );

  // Lint → Translator comment has extra placeholder(s): 6.

This PR adds support for this to prevent these false positive flags and precision placeholder supports.

How?

This PR supports this by extending the REGEXP_COMMENT_PLACEHOLDER Regex to support precisions as well as forcing a space after colon for placeholder.

The Regex now enforces groups for easier accessing values from matches.
Currently named and positional groups aren't being used in extra but it can be used in future depending on the use-case.

Testing Instructions

CI Lint Testing should suffice.

Screenshots or screencast

image

@USERSATOSHI USERSATOSHI marked this pull request as ready for review August 8, 2025 18:22
@github-actions
Copy link

github-actions bot commented Aug 8, 2025 •
edited
Loading

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: USERSATOSHI <tusharbharti@git.wordpress.org>
Co-authored-by: im3dabasia <im3dabasia1@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@t-hamano t-hamano added [Tool] ESLint plugin /packages/eslint-plugin [Type] Enhancement A suggestion for improvement. Internationalization (i18n) Issues or PRs related to internationalization efforts labels Aug 9, 2025
im3dabasia
im3dabasia reviewed Aug 11, 2025
View reviewed changes
Copy link
Contributor

@im3dabasia im3dabasia left a comment

Choose a reason for hiding this comment

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

Implementation looks good. However someone more versed in regex should confirm accuracy.

packages/eslint-plugin/rules/__tests__/i18n-translator-comments.js Outdated
code: ` // translators: %s: Hello at 6:00 AM
i18n.sprintf( i18n.__( 'Hello at %s' ), '6:00 AM' );`,
},
// test for precisions now
Copy link
Contributor

@im3dabasia im3dabasia Aug 11, 2025

Choose a reason for hiding this comment

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

I think this is not needed

Copy link
Contributor Author

@USERSATOSHI USERSATOSHI Aug 11, 2025

Choose a reason for hiding this comment

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

I added this test because it was mention that, this was causing a false error flag on extra placeholder

Link: #70458 (comment)

Copy link
Contributor

@im3dabasia im3dabasia Aug 11, 2025

Choose a reason for hiding this comment

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

I meant the inline comment 😅

// test for precisions now

Copy link
Contributor Author

@USERSATOSHI USERSATOSHI Aug 11, 2025

Choose a reason for hiding this comment

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

oh, my bad 😅

packages/eslint-plugin/utils/constants.js
* g — global, so it matches all placeholders in the comment string.
* ```
*/
const REGEXP_COMMENT_PLACEHOLDER =
Copy link
Contributor

@im3dabasia im3dabasia Aug 11, 2025

Choose a reason for hiding this comment

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

I feel a top-level arrow-based breakdown (similar to the one in REGEXP_SPRINTF_PLACEHOLDER example) would improve readability for the regex. I think the detailed explanation (if needed) could still remain at the bottom.

Having those quick visual markers at the top makes it much easier to parse at a glance. WDYT?

gutenberg/packages/eslint-plugin/utils/constants.js

Lines 39 to 46 in e10b175

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)

Copy link
Contributor Author

@USERSATOSHI USERSATOSHI Aug 11, 2025

Choose a reason for hiding this comment

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

I did also thought of that but when I tried to write that due to the regex length, the visual marker syntax felt awkward to read as it was getting hard to mark the points that I wanted to explain, Hence I took this approach as it allowed me to break the regex into a way, I thought I could explain it

@USERSATOSHI
Copy link
Contributor Author

USERSATOSHI commented Aug 20, 2025

Hi @swissspidy, I would also like opinion on this as it is related to previous eslint for stricter translator comments eslint.

@swissspidy swissspidy merged commit 5e757c1 into WordPress:trunk Aug 21, 2025
68 checks passed
@github-actions github-actions bot added this to the Gutenberg 21.6 milestone Aug 21, 2025
@maxschmeling maxschmeling mentioned this pull request Oct 7, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@im3dabasia im3dabasia im3dabasia left review comments

@swissspidy swissspidy swissspidy approved these changes

@gziolo gziolo Awaiting requested review from gziolo

@ntwb ntwb Awaiting requested review from ntwb ntwb is a code owner

@nerrad nerrad Awaiting requested review from nerrad nerrad is a code owner

@ajitbohra ajitbohra Awaiting requested review from ajitbohra ajitbohra is a code owner

Assignees

No one assigned

Labels

Internationalization (i18n) Issues or PRs related to internationalization efforts [Tool] ESLint plugin /packages/eslint-plugin [Type] Enhancement A suggestion for improvement.

Projects

None yet

Milestone

Gutenberg 21.6

Development

Successfully merging this pull request may close these issues.

4 participants

@USERSATOSHI @swissspidy @im3dabasia @t-hamano