| CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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.
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
Add support for precision type placeholders to translator comments eslint #71145
Conversation
USERSATOSHI
requested review from
ajitbohra,
gziolo,
nerrad and
ntwb
as code owners
|
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
t-hamano
added
[Tool] ESLint plugin
There was a problem hiding this 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.
| code: ` // translators: %s: Hello at 6:00 AM | ||
| i18n.sprintf( i18n.__( 'Hello at %s' ), '6:00 AM' );`, | ||
| }, | ||
| // test for precisions now |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, my bad 😅
| * g — global, so it matches all placeholders in the comment string. | ||
| * ``` | ||
| */ | ||
| const REGEXP_COMMENT_PLACEHOLDER = |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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
|
Hi @swissspidy, I would also like opinion on this as it is related to previous eslint for stricter translator comments eslint. |
Labels
4 participants
What?
Addresses:
comment#70458This 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$.2fetc.Example
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_PLACEHOLDERRegex 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